Skip to content

Timeseries: Epoch time instead of null for grandTotal.#5659

Closed
gianm wants to merge 1 commit intoapache:masterfrom
gianm:grandTotal-no-nulls
Closed

Timeseries: Epoch time instead of null for grandTotal.#5659
gianm wants to merge 1 commit intoapache:masterfrom
gianm:grandTotal-no-nulls

Conversation

@gianm
Copy link
Copy Markdown
Contributor

@gianm gianm commented Apr 17, 2018

May be friendlier to people who have written serde code that
assumes all timestamps are non-null.

This change was suggested by @b-slim on the dev sync today.

Follow up to #5640.

May be friendlier to people who have written serde code that
assumes all timestamps are non-null.
@gianm gianm requested a review from b-slim April 17, 2018 19:51
@jihoonson
Copy link
Copy Markdown
Contributor

What do you think about making this configurable? I think Epoch time is not much intuitive. Also, even though I hope no one is loading data of ‘1970-01-01T00:00:00Z’ timestamp, but it might be an issue if someone does.

@b-slim
Copy link
Copy Markdown
Contributor

b-slim commented Apr 17, 2018

@jihoonson good point, but am afraid this this will make code more complex for a small gain. Am okay with either ways.

Copy link
Copy Markdown
Contributor

@b-slim b-slim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, @jihoonson has good point worth another round.

@jihoonson
Copy link
Copy Markdown
Contributor

I'm not sure about how much this change benefits to users. But, at least in SQL world, grouping keys should be null if they cannot be determined. With this patch, I guess we should add a transform code to map the Epoch time to null to somewhere to support this feature in Druid's SQL as well. I think it would be better to make the unknown value configurable rather than adding such a transform.

@gianm
Copy link
Copy Markdown
Contributor Author

gianm commented Apr 18, 2018

Hmm. You're right about the SQL point - I forgot about that - but null will make this more compatible with GROUPING SETS. In light of that I think I should close this PR and keep it the way it is.

@b-slim what do you think?

@b-slim
Copy link
Copy Markdown
Contributor

b-slim commented Apr 18, 2018

am okay with keeping it null. But am wondering how null is more suited for SQL. In my mind i thought the timestamp is ignored anyway from the results set since it is not part of the row signature. So let say user issue select count(*) it gets translated to Timeseries with Granularity all and count aggregator even tho Druid returns an event that has a timestamp, the result parser does ignore it anyway, in other words timestamp is not part of the grouping key once we do the grandTotal aggregate. am i missing something here?

@gianm
Copy link
Copy Markdown
Contributor Author

gianm commented Apr 18, 2018

With granularity all the time stamp isn’t part of the grouping key, but it is for any other granularity. In that case, for a SQL query with a totals row, the time stamp should be null in that totals row (for example like in https://blogs.technet.microsoft.com/benjamin/2017/06/13/sql-tip-creating-a-grand-total-and-additional-subtotals/).

@b-slim
Copy link
Copy Markdown
Contributor

b-slim commented Apr 18, 2018

@gianm thanks i was not aware of that then we should use null

@gianm
Copy link
Copy Markdown
Contributor Author

gianm commented Apr 18, 2018

Closing this then. Thanks for reviewing @b-slim & @jihoonson

@gianm gianm closed this Apr 18, 2018
@gianm gianm deleted the grandTotal-no-nulls branch April 18, 2018 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants