[Feature] Support Coalesce for Column Names #9327

61yao · 2022-09-02T03:10:52Z

Coalesce takes a list of arguments and returns the first not null value. If all arguments are null, return a null.

This implementations transform all arguments into string and return the first non-null string value. Null is represented as string value "null"

codecov-commenter · 2022-09-02T03:50:29Z

Codecov Report

Merging #9327 (43ac34c) into master (ff2a333) will increase coverage by 3.56%.
The diff coverage is 87.50%.

@@             Coverage Diff              @@
##             master    #9327      +/-   ##
============================================
+ Coverage     63.40%   66.97%   +3.56%     
- Complexity     4762     4894     +132     
============================================
  Files          1832     1403     -429     
  Lines         98146    73108   -25038     
  Branches      15020    11722    -3298     
============================================
- Hits          62231    48964   -13267     
+ Misses        31321    20582   -10739     
+ Partials       4594     3562    -1032

Flag	Coverage Δ
unittests1	`66.97% <87.50%> (+0.05%)`	⬆️
unittests2	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
.../transform/function/CoalesceTransformFunction.java	`87.35% <87.35%> (ø)`
...e/pinot/common/function/TransformFunctionType.java	`100.00% <100.00%> (ø)`
...r/transform/function/TransformFunctionFactory.java	`86.89% <100.00%> (+0.09%)`	⬆️
.../org/apache/pinot/query/routing/WorkerManager.java	`83.87% <0.00%> (-6.87%)`	⬇️
...he/pinot/segment/local/segment/store/IndexKey.java	`65.00% <0.00%> (-5.00%)`	⬇️
.../aggregation/function/ModeAggregationFunction.java	`88.10% <0.00%> (ø)`
...ller/recommender/data/generator/TimeGenerator.java
...ontroller/api/resources/PinotControllerLogger.java
...inion/event/DefaultMinionEventObserverFactory.java
...ker/routing/instanceselector/InstanceSelector.java
... and 430 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

...c/main/java/org/apache/pinot/core/operator/transform/function/CoalesceTransformFunction.java

siddharthteotia · 2022-09-07T09:26:45Z

...c/main/java/org/apache/pinot/core/operator/transform/function/CoalesceTransformFunction.java

+  }
+
+  @Override
+  public String[] transformToStringValuesSV(ProjectionBlock projectionBlock) {


I think implicitly treating everything as STRING will give confusing semantics imo. If we don't want to support this function on few data types, then throwing exception is better.

Reference - https://docs.snowflake.com/en/sql-reference/functions/coalesce.html

+1. For the first version, we can check if all the input are numbers or strings

I added a check for argument type. but the return value has to be a string, otherwise we are not able to represent null?

Good point. Currently we don't support null values for transform function yet. You may read TransformBlockValSet.getNullBitmap(), which count the result as null when any argument is null. This won't work for coalesce.

For now, we may use NullValueUtils.getDefaultNullValue() for each data type to present the null. Then we should figure out a way to support the real null in transform function.

(edited) represented using default nullvalue probably is not a good idea since all numeric data type uses zero as default null.

@walterddr Not really. We use min value as the default for numeric types except for big decimal, which doesn't have a min value

Updated to use NullValueUtils.getDefaultNullValue() and only supports numeric and string types. Also it requires the type to be same for all arguments.

...c/main/java/org/apache/pinot/core/operator/transform/function/CoalesceTransformFunction.java

Jackie-Jiang · 2022-09-13T07:27:52Z

...c/main/java/org/apache/pinot/core/operator/transform/function/CoalesceTransformFunction.java

+      Preconditions.checkArgument(func instanceof IdentifierTransformFunction,
+          "Only column names are supported in COALESCE.");
+      FieldSpec.DataType dataType = func.getResultMetadata().getDataType().getStoredType();
+      Preconditions.checkArgument(dataType.isNumeric() || dataType == FieldSpec.DataType.STRING,


I don't think we need this check. We may use getResultMetadata() to rule out the unsupported types

Removed the second check but we need the type to be identifier to get the null bit map?

...c/main/java/org/apache/pinot/core/operator/transform/function/CoalesceTransformFunction.java

Jackie-Jiang

LGTM with minor comments

Jackie-Jiang · 2022-09-14T03:30:27Z

...c/main/java/org/apache/pinot/core/operator/transform/function/CoalesceTransformFunction.java

+      TransformFunction func = arguments.get(i);
+      Preconditions.checkArgument(func instanceof IdentifierTransformFunction,
+          "Only column names are supported in COALESCE.");
+      FieldSpec.DataType dataType = func.getResultMetadata().getDataType().getStoredType();


(minor) Let's change the name to storedType to be more explicit

Jackie-Jiang · 2022-09-14T03:32:40Z

...c/main/java/org/apache/pinot/core/operator/transform/function/CoalesceTransformFunction.java

+  @Override
+  public TransformResultMetadata getResultMetadata() {
+    switch (_dataType) {
+      case STRING:


(minor) We usually follow the sequence of INT, LONG, FLOAT, DOUBLE, BIG_DECIMAL, STRING (same order as the enum for easier tracking

(also use intellij autocomplete to generate the branches will automatically be in that order :-) )

Jackie-Jiang · 2022-09-14T03:33:57Z

...c/main/java/org/apache/pinot/core/operator/transform/function/CoalesceTransformFunction.java

+  }
+
+  @Override
+  public TransformResultMetadata getResultMetadata() {


(minor) Let's calculate the result metadata in the init() and store it in a member variable. It has 2 benefits:

Fail fast when the type cannot be supported

Prevent calculating result metadata multiple times

Jackie-Jiang · 2022-09-14T03:35:21Z

...c/main/java/org/apache/pinot/core/operator/transform/function/CoalesceTransformFunction.java

+  }
+
+  @Override
+  public String[] transformToStringValuesSV(ProjectionBlock projectionBlock) {


(minor) Follow the same sequence (INT, LONG, FLOAT, DOUBLE, BIG_DECIMAL, STRING) as the interface for easier tracking

Jackie-Jiang · 2022-09-14T03:35:45Z

...c/main/java/org/apache/pinot/core/operator/transform/function/CoalesceTransformFunction.java

+   * Get transform float results based on store type.
+   * @param projectionBlock
+   */
+  private float[] getFloatTransformResults(ProjectionBlock projectionBlock) {


(minor) Put this method before getDoublelTransformResults() for easier tracking

siddharthteotia reviewed Sep 7, 2022

View reviewed changes

...c/main/java/org/apache/pinot/core/operator/transform/function/CoalesceTransformFunction.java Outdated Show resolved Hide resolved

siddharthteotia reviewed Sep 7, 2022

View reviewed changes

61yao force-pushed the coalesce branch 3 times, most recently from adbc410 to 21d21ae Compare September 10, 2022 01:19

Jackie-Jiang reviewed Sep 13, 2022

View reviewed changes

Jackie-Jiang approved these changes Sep 14, 2022

View reviewed changes

Yao Liu added 5 commits September 14, 2022 14:53

test

959d32c

coalesce

814c237

use bitmap from projection and check argument types

407c29f

Handle different types of Coalesce

0aa8d30

add double float support

015d94f

61yao force-pushed the coalesce branch from fb3c559 to 7c80923 Compare September 14, 2022 21:53

address comments

43ac34c

61yao force-pushed the coalesce branch from 7c80923 to 43ac34c Compare September 14, 2022 21:57

Merge branch 'master' into coalesce

69589be

walterddr merged commit a949f95 into apache:master Sep 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] Support Coalesce for Column Names #9327

[Feature] Support Coalesce for Column Names #9327

61yao commented Sep 2, 2022

codecov-commenter commented Sep 2, 2022 •

edited

siddharthteotia Sep 7, 2022

Jackie-Jiang Sep 7, 2022

61yao Sep 8, 2022

Jackie-Jiang Sep 8, 2022

walterddr Sep 9, 2022 •

edited

Jackie-Jiang Sep 9, 2022

61yao Sep 10, 2022

Jackie-Jiang Sep 13, 2022

61yao Sep 13, 2022

Jackie-Jiang left a comment

Jackie-Jiang Sep 14, 2022

Jackie-Jiang Sep 14, 2022

walterddr Sep 14, 2022

Jackie-Jiang Sep 14, 2022

walterddr Sep 14, 2022

Jackie-Jiang Sep 14, 2022

Jackie-Jiang Sep 14, 2022

[Feature] Support Coalesce for Column Names #9327

[Feature] Support Coalesce for Column Names #9327

Conversation

61yao commented Sep 2, 2022

codecov-commenter commented Sep 2, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

walterddr Sep 9, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 2, 2022 •

edited

walterddr Sep 9, 2022 •

edited