Fix four bugs with numeric dimension output types. by gianm · Pull Request #6220 · apache/druid

gianm · 2018-08-22T22:45:23Z

This patch includes the following bug fixes:

TopNColumnSelectorStrategyFactory: Cast dimension values to the output type
during dimExtractionScanAndAggregate instead of updateDimExtractionResults.
This fixes a bug where, for example, grouping on doubles-cast-to-longs would
fail to merge two doubles that should have been combined into the same long value.
TopNQueryEngine: Use DimExtractionTopNAlgorithm when treating string columns
as numeric dimensions. This fixes a similar bug: grouping on string-cast-to-long
would fail to merge two strings that should have been combined.
GroupByQuery: Cast numeric types to the expected output type before comparing them
in compareDimsForLimitPushDown. This fixes ClassCastException in groupBy when sorting on numeric columns containing nulls #6123.
GroupByQueryQueryToolChest: Convert Jackson-deserialized dimension values into
the proper output type. This fixes an inconsistency between results that came
from cache vs. not-cache: for example, Jackson sometimes deserializes integers
as Integers and sometimes as Longs.

And the following code-cleanup changes, related to the fixes above:

DimensionHandlerUtils: Introduce convertObjectToType, compareObjectsAsType,
and converterFromTypeToType to make it easier to handle casting operations.
TopN in general: Rename various "dimName" variables to "dimValue" where they
actually represent dimension values. The old names were confusing.

This patch includes the following bug fixes: - TopNColumnSelectorStrategyFactory: Cast dimension values to the output type during dimExtractionScanAndAggregate instead of updateDimExtractionResults. This fixes a bug where, for example, grouping on doubles-cast-to-longs would fail to merge two doubles that should have been combined into the same long value. - TopNQueryEngine: Use DimExtractionTopNAlgorithm when treating string columns as numeric dimensions. This fixes a similar bug: grouping on string-cast-to-long would fail to merge two strings that should have been combined. - GroupByQuery: Cast numeric types to the expected output type before comparing them in compareDimsForLimitPushDown. This fixes apache#6123. - GroupByQueryQueryToolChest: Convert Jackson-deserialized dimension values into the proper output type. This fixes an inconsistency between results that came from cache vs. not-cache: for example, Jackson sometimes deserializes integers as Integers and sometimes as Longs. And the following code-cleanup changes, related to the fixes above: - DimensionHandlerUtils: Introduce convertObjectToType, compareObjectsAsType, and converterFromTypeToType to make it easier to handle casting operations. - TopN in general: Rename various "dimName" variables to "dimValue" where they actually represent dimension values. The old names were confusing.

clintropolis

nice 🤘

leventov · 2018-08-24T04:02:41Z

-        }
-
-
+        // Output type must be STRING in order for PooledTopNAlgorithm to make sense; so no need to convert value.


Maybe add an assert about the output type here.

leventov · 2018-08-24T04:16:36Z

 {
-  @Override
-  public int getCardinality(DimensionSelector selector)
+  private final Function<Object, Comparable<?>> dimensionValueConverter;


Function from String?

leventov · 2018-08-24T04:17:12Z

      DimensionSelector selector,
      Aggregator[][] rowSelector,
-      Map<String, Aggregator[]> aggregatesStore
+      Map<Comparable, Aggregator[]> aggregatesStore


Comparable and Comparable<?> are used inconsistently in this class

leventov · 2018-08-24T04:30:29Z

-        return new NumericTopNColumnSelectorStrategy.OfDouble();
+        if (ValueType.isNumeric(dimensionType)) {
+          // Return strategy that aggregates using the _output_ type, because this allows us to collapse values
+          // properly (numeric types cannot represent all values of other numeric types).


FWIW double represents all values of float

leventov · 2018-08-24T04:31:14Z

+          // properly (numeric types cannot represent all values of other numeric types).
+          return NumericTopNColumnSelectorStrategy.ofType(dimensionType, dimensionType);
+        } else {
+          // Return strategy that aggregates using the _input_ type. Here we are assuming that the output type can


Despite those comments, it's still hard to understand what is going on here. Maybe you could try to clarify it more?

leventov · 2018-08-24T04:33:02Z

+
+  @Nullable
+  public static Comparable<?> convertObjectToType(
+      @Nullable final Object obj,


Unnecessary breakdown

* Fix four bugs with numeric dimension output types. This patch includes the following bug fixes: - TopNColumnSelectorStrategyFactory: Cast dimension values to the output type during dimExtractionScanAndAggregate instead of updateDimExtractionResults. This fixes a bug where, for example, grouping on doubles-cast-to-longs would fail to merge two doubles that should have been combined into the same long value. - TopNQueryEngine: Use DimExtractionTopNAlgorithm when treating string columns as numeric dimensions. This fixes a similar bug: grouping on string-cast-to-long would fail to merge two strings that should have been combined. - GroupByQuery: Cast numeric types to the expected output type before comparing them in compareDimsForLimitPushDown. This fixes apache#6123. - GroupByQueryQueryToolChest: Convert Jackson-deserialized dimension values into the proper output type. This fixes an inconsistency between results that came from cache vs. not-cache: for example, Jackson sometimes deserializes integers as Integers and sometimes as Longs. And the following code-cleanup changes, related to the fixes above: - DimensionHandlerUtils: Introduce convertObjectToType, compareObjectsAsType, and converterFromTypeToType to make it easier to handle casting operations. - TopN in general: Rename various "dimName" variables to "dimValue" where they actually represent dimension values. The old names were confusing. * Remove unused imports.

leventov · 2018-08-25T23:34:39Z

@fjy please don't merge PRs with unanswered comments (despite the approval), otherwise why they are left?

gianm · 2018-08-25T23:44:45Z

Fwiw, I had already started addressing some of those comments, so I plan to raise a follow up PR for that.

Btw, @leventov what did you intend by doing an approval + also leaving comments? I'd interpret that as "please consider these comments, but if you don't want to do them, I am ok with that." I thought they were all reasonable comments so that's why I'm doing a follow up.

Adjustments to comments and usage of generics.

gianm · 2018-08-25T23:52:27Z

Follow-up is in #6231.

leventov · 2018-08-26T00:17:36Z

Thanks.

Yes it means non-blocking comments, however answering them before merging is appreciated.

* Fix four bugs with numeric dimension output types. This patch includes the following bug fixes: - TopNColumnSelectorStrategyFactory: Cast dimension values to the output type during dimExtractionScanAndAggregate instead of updateDimExtractionResults. This fixes a bug where, for example, grouping on doubles-cast-to-longs would fail to merge two doubles that should have been combined into the same long value. - TopNQueryEngine: Use DimExtractionTopNAlgorithm when treating string columns as numeric dimensions. This fixes a similar bug: grouping on string-cast-to-long would fail to merge two strings that should have been combined. - GroupByQuery: Cast numeric types to the expected output type before comparing them in compareDimsForLimitPushDown. This fixes apache#6123. - GroupByQueryQueryToolChest: Convert Jackson-deserialized dimension values into the proper output type. This fixes an inconsistency between results that came from cache vs. not-cache: for example, Jackson sometimes deserializes integers as Integers and sometimes as Longs. And the following code-cleanup changes, related to the fixes above: - DimensionHandlerUtils: Introduce convertObjectToType, compareObjectsAsType, and converterFromTypeToType to make it easier to handle casting operations. - TopN in general: Rename various "dimName" variables to "dimValue" where they actually represent dimension values. The old names were confusing. * Remove unused imports.

* Fix four bugs with numeric dimension output types. This patch includes the following bug fixes: - TopNColumnSelectorStrategyFactory: Cast dimension values to the output type during dimExtractionScanAndAggregate instead of updateDimExtractionResults. This fixes a bug where, for example, grouping on doubles-cast-to-longs would fail to merge two doubles that should have been combined into the same long value. - TopNQueryEngine: Use DimExtractionTopNAlgorithm when treating string columns as numeric dimensions. This fixes a similar bug: grouping on string-cast-to-long would fail to merge two strings that should have been combined. - GroupByQuery: Cast numeric types to the expected output type before comparing them in compareDimsForLimitPushDown. This fixes #6123. - GroupByQueryQueryToolChest: Convert Jackson-deserialized dimension values into the proper output type. This fixes an inconsistency between results that came from cache vs. not-cache: for example, Jackson sometimes deserializes integers as Integers and sometimes as Longs. And the following code-cleanup changes, related to the fixes above: - DimensionHandlerUtils: Introduce convertObjectToType, compareObjectsAsType, and converterFromTypeToType to make it easier to handle casting operations. - TopN in general: Rename various "dimName" variables to "dimValue" where they actually represent dimension values. The old names were confusing. * Remove unused imports.

Adjustments to comments and usage of generics.

…che#6230) * Fix four bugs with numeric dimension output types. This patch includes the following bug fixes: - TopNColumnSelectorStrategyFactory: Cast dimension values to the output type during dimExtractionScanAndAggregate instead of updateDimExtractionResults. This fixes a bug where, for example, grouping on doubles-cast-to-longs would fail to merge two doubles that should have been combined into the same long value. - TopNQueryEngine: Use DimExtractionTopNAlgorithm when treating string columns as numeric dimensions. This fixes a similar bug: grouping on string-cast-to-long would fail to merge two strings that should have been combined. - GroupByQuery: Cast numeric types to the expected output type before comparing them in compareDimsForLimitPushDown. This fixes apache#6123. - GroupByQueryQueryToolChest: Convert Jackson-deserialized dimension values into the proper output type. This fixes an inconsistency between results that came from cache vs. not-cache: for example, Jackson sometimes deserializes integers as Integers and sometimes as Longs. And the following code-cleanup changes, related to the fixes above: - DimensionHandlerUtils: Introduce convertObjectToType, compareObjectsAsType, and converterFromTypeToType to make it easier to handle casting operations. - TopN in general: Rename various "dimName" variables to "dimValue" where they actually represent dimension values. The old names were confusing. * Remove unused imports.

Similar to other bugs fixed in apache#6220, but this one was missed. This bug would cause "extraction" dimensionSpecs on the "__time" column with non-STRING outputTypes to potentially be output as STRING sometimes instead of LONG, causing incompletely merged results.

Similar to other bugs fixed in #6220, but this one was missed. This bug would cause "extraction" dimensionSpecs on the "__time" column with non-STRING outputTypes to potentially be output as STRING sometimes instead of LONG, causing incompletely merged results.

Similar to other bugs fixed in apache#6220, but this one was missed. This bug would cause "extraction" dimensionSpecs on the "__time" column with non-STRING outputTypes to potentially be output as STRING sometimes instead of LONG, causing incompletely merged results.

Similar to other bugs fixed in #6220, but this one was missed. This bug would cause "extraction" dimensionSpecs on the "__time" column with non-STRING outputTypes to potentially be output as STRING sometimes instead of LONG, causing incompletely merged results.

gianm added Bug Area - Querying labels Aug 22, 2018

gianm added this to the 0.12.3 milestone Aug 22, 2018

Remove unused imports.

6d1bff3

clintropolis approved these changes Aug 24, 2018

View reviewed changes

leventov approved these changes Aug 24, 2018

View reviewed changes

fjy merged commit 23ba6f7 into apache:master Aug 25, 2018

gianm deleted the fix-some-numeric-dim-stuff branch August 25, 2018 23:31

gianm added a commit to gianm/druid that referenced this pull request Aug 25, 2018

Minor followup to apache#6220.

c91b26b

Adjustments to comments and usage of generics.

leventov pushed a commit that referenced this pull request Aug 27, 2018

Minor followup to #6220. (#6231)

157e75a

Adjustments to comments and usage of generics.

jon-wei mentioned this pull request Sep 1, 2018

[DRAFT] Druid 0.12.3 release notes #6288

Closed

gianm mentioned this pull request Mar 13, 2019

Fix time-extraction topN with non-STRING outputType. #7257

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix four bugs with numeric dimension output types.#6220

Fix four bugs with numeric dimension output types.#6220
fjy merged 2 commits intoapache:masterfrom
gianm:fix-some-numeric-dim-stuff

gianm commented Aug 22, 2018

Uh oh!

clintropolis left a comment

Uh oh!

leventov Aug 24, 2018

Uh oh!

leventov Aug 24, 2018

Uh oh!

leventov Aug 24, 2018

Uh oh!

leventov Aug 24, 2018

Uh oh!

leventov Aug 24, 2018

Uh oh!

leventov Aug 24, 2018

Uh oh!

leventov commented Aug 25, 2018

Uh oh!

gianm commented Aug 25, 2018

Uh oh!

gianm commented Aug 25, 2018

Uh oh!

leventov commented Aug 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		}


		// Output type must be STRING in order for PooledTopNAlgorithm to make sense; so no need to convert value.

Conversation

gianm commented Aug 22, 2018

Uh oh!

clintropolis left a comment

Choose a reason for hiding this comment

Uh oh!

leventov Aug 24, 2018

Choose a reason for hiding this comment

Uh oh!

leventov Aug 24, 2018

Choose a reason for hiding this comment

Uh oh!

leventov Aug 24, 2018

Choose a reason for hiding this comment

Uh oh!

leventov Aug 24, 2018

Choose a reason for hiding this comment

Uh oh!

leventov Aug 24, 2018

Choose a reason for hiding this comment

Uh oh!

leventov Aug 24, 2018

Choose a reason for hiding this comment

Uh oh!

leventov commented Aug 25, 2018

Uh oh!

gianm commented Aug 25, 2018

Uh oh!

gianm commented Aug 25, 2018

Uh oh!

leventov commented Aug 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants