Skip to content

Support double type metric#3294

Closed
navis wants to merge 2 commits intoapache:masterfrom
navis:support-double-metric
Closed

Support double type metric#3294
navis wants to merge 2 commits intoapache:masterfrom
navis:support-double-metric

Conversation

@navis
Copy link
Copy Markdown
Contributor

@navis navis commented Jul 27, 2016

We suffered some precision problems of metrics because currently it's allowed to be one of float or long type. This patch is for supporting double type metric in druid. I'm expecting BigDecimal type could be considered for next target.

@navis navis force-pushed the support-double-metric branch 2 times, most recently from acd01a6 to 92d6016 Compare July 28, 2016 08:25
@gianm
Copy link
Copy Markdown
Contributor

gianm commented Jul 30, 2016

@navis, this looks really useful! Haven't read everything yet, but initial comments:

  • why not always use a DoubleColumnSelector for the DoubleSumAggregator? It's going to need to get cast into a double and added into a double anyway.
  • looks at indexing time, inputType of the generic aggregator factories determines the type of the column stored on disk. this is a bit counter-intuitive! maybe something neutral like valueType would be more intuitive.
  • are you expecting we would deprecate {double,long}{min,max,sum} aggregator factories in favor of the "generic" versions?
  • are you using this in production?

@navis
Copy link
Copy Markdown
Contributor Author

navis commented Jul 30, 2016

@gianm

  • it's generally acceptable to use just double selector as input especially when the columns is already a numeric value in input row but when it's string and should be parsed into, Long.valueOf is way lighter than Double.valueOf (10x on cpu time) and also possible to loose some precisions.
  • inputType is from type of column selector at first. I'm ok whatever the name would be.
  • I didn't think about that. should we keep it for compatibility?
  • we are using DoubleColumnSelector modified to accept double input and store it as double. I regret that because it will be a problem someday.

@navis navis force-pushed the support-double-metric branch 2 times, most recently from b29addf to a3a9211 Compare August 2, 2016 02:07
@navis navis force-pushed the support-double-metric branch from a3a9211 to af8b105 Compare August 23, 2016 04:37
@navis navis force-pushed the support-double-metric branch 2 times, most recently from 0e447db to 94e1d75 Compare October 13, 2016 00:22
@b-slim
Copy link
Copy Markdown
Contributor

b-slim commented Jun 5, 2017

@navis and @fjy are you guys still working on this ?
We also have some issue with precision lose and would like to see this Done. I can help reviewing or taking from you guys have stopped.
Please let me know ASAP thanks.

@dylwylie
Copy link
Copy Markdown
Contributor

dylwylie commented Feb 7, 2019

Closing as I believe this was covered in #4491

@dylwylie dylwylie closed this Feb 7, 2019
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.

5 participants