You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "abhioncbr (via GitHub)" <gi...@apache.org> on 2023/06/03 19:04:37 UTC
[GitHub] [pinot] abhioncbr opened a new pull request, #10814: [multistage][float-double-comparison]: updated for CAST function addition based on correct precision.
abhioncbr opened a new pull request, #10814:
URL: https://github.com/apache/pinot/pull/10814
As per the discussion on [issue](https://github.com/apache/pinot/issues/10686), updated the code for handling the float-double comparison.
~~- Avoiding the right `CAST` transformation function, reading the data as `Double` and performing the comparison.~~
- override the `getDefaultPrecision` method of the class `org.apache.calcite.rel.type.RelDataTypeSystemImpl` for having different Float and Double precision values.
cc: @Jackie-Jiang @walterddr
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhioncbr commented on pull request #10814: [multistage][float-double-comparison]: updated for CAST function addition based on correct precision.
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on PR #10814:
URL: https://github.com/apache/pinot/pull/10814#issuecomment-1580715489
@walterddr I saw you have tried to build the PR. Just checking is it acceptable line of solution or we need to look `TypeCoercion` logic?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on pull request #10814: [multistage][float-double-comparison]: updated for CAST function addition based on correct precision.
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on PR #10814:
URL: https://github.com/apache/pinot/pull/10814#issuecomment-1583063933
>
yes there were some issue with the latest master. fixed just yesterday. will review again now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #10814: [multistage][float-double-comparison]: updated for CAST function addition based on correct precision.
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10814:
URL: https://github.com/apache/pinot/pull/10814#discussion_r1228464036
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/planner/logical/RelToPlanNodeConverter.java:
##########
@@ -209,8 +209,8 @@ public static DataSchema.ColumnDataType convertToColumnDataType(RelDataType relD
case DECIMAL:
return resolveDecimal(relDataType);
case FLOAT:
- return isArray ? DataSchema.ColumnDataType.FLOAT_ARRAY : DataSchema.ColumnDataType.FLOAT;
case REAL:
+ return isArray ? DataSchema.ColumnDataType.FLOAT_ARRAY : DataSchema.ColumnDataType.FLOAT;
Review Comment:
let's add a comment --> this is where we convert from Calcite's REAL data type to Pinot's FLOAT data type
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/type/TypeFactory.java:
##########
@@ -60,8 +60,8 @@ private RelDataType toRelDataType(FieldSpec fieldSpec) {
return fieldSpec.isSingleValueField() ? createSqlType(SqlTypeName.BIGINT)
: createArrayType(createSqlType(SqlTypeName.BIGINT), -1);
case FLOAT:
- return fieldSpec.isSingleValueField() ? createSqlType(SqlTypeName.FLOAT)
- : createArrayType(createSqlType(SqlTypeName.FLOAT), -1);
+ return fieldSpec.isSingleValueField() ? createSqlType(SqlTypeName.REAL)
+ : createArrayType(createSqlType(SqlTypeName.REAL), -1);
Review Comment:
add comments: this is where we convert the Pinot's FLOAT data type to Calcite's REAL data type, b/c calcite's FLOAT is actually "DOUBLE", we have to make sure calcite understands the precision/scale configuration of Pinot's FLOAT is actually Calcite's REAL.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on pull request #10814: [multistage][float-double-comparison]: updated for CAST function addition based on correct precision.
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on PR #10814:
URL: https://github.com/apache/pinot/pull/10814#issuecomment-1573902723
according to `TypeCoercion` logic it should automatically hoist both INT-LONG and FLOAT-DOUBLE in Calcite.
it is also possible the cast is there for null vs. non-null which also will trigger a type coercion logic.
I will take a look and see what's the appropriate overwrite for this one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter commented on pull request #10814: 10686: updated for avoiding right cast transformation incase of float…
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10814:
URL: https://github.com/apache/pinot/pull/10814#issuecomment-1567325042
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/10814?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#10814](https://app.codecov.io/gh/apache/pinot/pull/10814?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (150428c) into [master](https://app.codecov.io/gh/apache/pinot/commit/51bf75efa65cbe8bd8b497eb20e34869205d74e8?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (51bf75e) will **decrease** coverage by `38.77%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #10814 +/- ##
=============================================
- Coverage 70.22% 31.45% -38.77%
+ Complexity 6525 453 -6072
=============================================
Files 2164 2164
Lines 116369 116381 +12
Branches 17599 17601 +2
=============================================
- Hits 81719 36613 -45106
- Misses 28952 76455 +47503
+ Partials 5698 3313 -2385
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `23.92% <0.00%> (-0.17%)` | :arrow_down: |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `13.63% <0.00%> (-0.02%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/pinot/pull/10814?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...form/function/BinaryOperatorTransformFunction.java](https://app.codecov.io/gh/apache/pinot/pull/10814?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vQmluYXJ5T3BlcmF0b3JUcmFuc2Zvcm1GdW5jdGlvbi5qYXZh) | `19.24% <0.00%> (-46.89%)` | :arrow_down: |
| [...ator/transform/function/CastTransformFunction.java](https://app.codecov.io/gh/apache/pinot/pull/10814?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vQ2FzdFRyYW5zZm9ybUZ1bmN0aW9uLmphdmE=) | `18.35% <0.00%> (-41.16%)` | :arrow_down: |
... and [1376 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/10814/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang merged pull request #10814: [multistage][float-double-comparison]: updated for CAST function addition based on correct precision.
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #10814:
URL: https://github.com/apache/pinot/pull/10814
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhioncbr commented on pull request #10814: [multistage][float-double-comparison]: updated for CAST function addition based on correct precision.
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on PR #10814:
URL: https://github.com/apache/pinot/pull/10814#issuecomment-1589611356
> this looks good to me. let's wait for the test to complete
I don't think integration test failure is related to these PR changes
```
Error: Failures:
Error: MultiNodesOfflineClusterIntegrationTest.testServerHardFailure:161 Failed to meet condition in 1000ms after 0 errors, error message: Failed to include the restarted server into the routing. Other tests may be affected
[INFO]
Error: Tests run: 115, Failures: 1, Errors: 0, Skipped: 0
```
I can possibly merge the branch with master and give it a try.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhioncbr commented on pull request #10814: [multistage][float-double-comparison]: updated for CAST function addition based on correct precision.
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on PR #10814:
URL: https://github.com/apache/pinot/pull/10814#issuecomment-1573927172
> according to `TypeCoercion` logic it should automatically hoist both INT-LONG and FLOAT-DOUBLE in Calcite. it is also possible the cast is there for null vs. non-null which also will trigger a type coercion logic.
>
> I will take a look and see what's the appropriate overwrite for this one.
I found that the CAST will happen based on the evaluation of this [function](https://github.com/apache/calcite/blob/main/core/src/main/java/org/apache/calcite/rel/type/RelDataTypeSystemImpl.java#L62). The `CAST` transformation applies to the data type with lower precision. In the case of INT-LONG, Integer has lower precision(10) vs BigInt(19). However, In the case of FLOAT-DOUBLE, both have the same precision and hence CAST applies to the RHS. I override the method in my latest commit by assigning `DOUBLE` more precision value than `FLOAT`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhioncbr closed pull request #10814: [multistage][float-double-comparison]: updated for CAST function addition based on correct precision.
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr closed pull request #10814: [multistage][float-double-comparison]: updated for CAST function addition based on correct precision.
URL: https://github.com/apache/pinot/pull/10814
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhioncbr commented on a diff in pull request #10814: float-double-comparison: updated for avoiding right cast transformation incase of float…
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on code in PR #10814:
URL: https://github.com/apache/pinot/pull/10814#discussion_r1210931175
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/BinaryOperatorTransformFunction.java:
##########
@@ -422,6 +422,11 @@ private void fillLongResultArray(ValueBlock valueBlock, float[] leftValues, int
}
private void fillFloatResultArray(ValueBlock valueBlock, float[] leftValues, int length) {
+ // handling float double comparison, skipping the right CAST function and evaluate the result.
Review Comment:
This solution can only handle float-double comparison in the `SELECT` clause. Not able to handle similar comparisons in the `WHERE` clause.
I am looking for a better approach to handle both scenarios.
@Jackie-Jiang @walterddr Do you think float-double comparison can occur in some other SQL clause as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhioncbr commented on pull request #10814: [multistage][float-double-comparison]: updated for CAST function addition based on correct precision.
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on PR #10814:
URL: https://github.com/apache/pinot/pull/10814#issuecomment-1574523914
I did some analysis, and here are a couple of points.
- when we create a numeric `RelDataType` by invoking the function [toRelDataType](https://github.com/apache/pinot/blob/master/pinot-query-planner/src/main/java/org/apache/pinot/query/type/TypeFactory.java#L54), none of them has precision or scale value. The reason being we always get a `False` value from the method [allowsPrec()](https://github.com/apache/calcite/blob/main/core/src/main/java/org/apache/calcite/sql/type/SqlTypeFactoryImpl.java#L51)
- when we invoke the [getPrecision()](https://github.com/apache/calcite/blob/main/core/src/main/java/org/apache/calcite/sql/type/BasicSqlType.java#L149) method for the Numeric data type, it returns the value based on ```typeSystem.getDefaultPrecision(typeName)```.
- I am not able to find any relationship between the method [decimalOf2()](https://github.com/apache/calcite/blob/main/core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactoryImpl.java#L586) in the `createSqlType` call stack.
With the above-mentioned points, i see we have two options
- Override the `getDefaultPrecision` in `TypeSystem` with something like that
```
@Override public int getDefaultPrecision(SqlTypeName typeName) {
switch (typeName) {
case DOUBLE:
return 16;
default:
return super.getDefaultPrecision(typeName)
}
}
```
- Invoking `decimalOf2()` in [toRelDataType()](https://github.com/apache/pinot/blob/master/pinot-query-planner/src/main/java/org/apache/pinot/query/type/TypeFactory.java#L54) for Numeric data type to set the precision and scale values.
@walterddr please suggest here. Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #10814: [multistage][float-double-comparison]: updated for CAST function addition based on correct precision.
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10814:
URL: https://github.com/apache/pinot/pull/10814#discussion_r1223361422
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/type/TypeSystem.java:
##########
@@ -70,4 +70,16 @@ public RelDataType deriveAvgAggType(RelDataTypeFactory typeFactory,
}
}
}
+
+ // overriding the function for handling comparison between different data types.
+ // based on the precision value calcite adds the CAST function to make operands of same type for comparison.
+ @Override public int getDefaultPrecision(SqlTypeName typeName) {
+ switch (typeName) {
+ case DOUBLE:
+ return 16;
+ default:
+ // Following BasicSqlType precision as the default
+ return super.getDefaultPrecision(typeName);
+ }
Review Comment:
i think this is the problem : in JavaTypeFactoryImpl
```
@Override public Type getJavaClass(RelDataType type) {
...
if (type instanceof BasicSqlType || type instanceof IntervalSqlType) {
switch (type.getSqlTypeName()) {
...
case DOUBLE:
case FLOAT: // sic
return type.isNullable() ? Double.class : double.class;
case REAL:
return type.isNullable() ? Float.class : float.class;
```
This means `SQL.REAL` is actually java Float. and `SQL.FLOAT` or `SQL.DOUBLE` is actually java Double...
This means all of our interpretation on float is wrong (b/c we equivalent `SQL.FLOAT` as java Float
This is a much bigger problem IMO
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr closed pull request #10814: [multistage][float-double-comparison]: updated for CAST function addition based on correct precision.
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr closed pull request #10814: [multistage][float-double-comparison]: updated for CAST function addition based on correct precision.
URL: https://github.com/apache/pinot/pull/10814
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #10814: [multistage][float-double-comparison]: updated for CAST function addition based on correct precision.
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10814:
URL: https://github.com/apache/pinot/pull/10814#discussion_r1224914165
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/type/TypeSystem.java:
##########
@@ -70,4 +70,16 @@ public RelDataType deriveAvgAggType(RelDataTypeFactory typeFactory,
}
}
}
+
+ // overriding the function for handling comparison between different data types.
+ // based on the precision value calcite adds the CAST function to make operands of same type for comparison.
+ @Override public int getDefaultPrecision(SqlTypeName typeName) {
+ switch (typeName) {
+ case DOUBLE:
+ return 16;
+ default:
+ // Following BasicSqlType precision as the default
+ return super.getDefaultPrecision(typeName);
+ }
Review Comment:
`SqlTypeFactoryImpl` uses `RelDataTypeSystemImpl` to compute the scale/precision, which behaves the same as JavaTypeFactoryImpl.
```
...
case REAL:
return 7;
case FLOAT:
case DOUBLE:
return 15;
```
what we should really do here is to change `TypeFactory` to :
```
case FLOAT:
return fieldSpec.isSingleValueField() ? createSqlType(SqlTypeName.REAL) // instead of FLOAT
: createArrayType(createSqlType(SqlTypeName.REAL), -1);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhioncbr commented on a diff in pull request #10814: float-double-comparison: updated for avoiding right cast transformation incase of float…
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on code in PR #10814:
URL: https://github.com/apache/pinot/pull/10814#discussion_r1210931175
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/BinaryOperatorTransformFunction.java:
##########
@@ -422,6 +422,11 @@ private void fillLongResultArray(ValueBlock valueBlock, float[] leftValues, int
}
private void fillFloatResultArray(ValueBlock valueBlock, float[] leftValues, int length) {
+ // handling float double comparison, skipping the right CAST function and evaluate the result.
Review Comment:
This solution can only handle float-double comparison in the `SELECT` clause. Not able to handle similar comparisons in the `WHERE` clause.
I am looking for a better approach to handle both scenarios.
@Jackie-Jiang @walterddr Do you think float-double comparison can occur in some other SQL clause?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #10814: [multistage][float-double-comparison]: updated for CAST function addition based on correct precision.
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10814:
URL: https://github.com/apache/pinot/pull/10814#discussion_r1224914165
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/type/TypeSystem.java:
##########
@@ -70,4 +70,16 @@ public RelDataType deriveAvgAggType(RelDataTypeFactory typeFactory,
}
}
}
+
+ // overriding the function for handling comparison between different data types.
+ // based on the precision value calcite adds the CAST function to make operands of same type for comparison.
+ @Override public int getDefaultPrecision(SqlTypeName typeName) {
+ switch (typeName) {
+ case DOUBLE:
+ return 16;
+ default:
+ // Following BasicSqlType precision as the default
+ return super.getDefaultPrecision(typeName);
+ }
Review Comment:
`SqlTypeFactoryImpl` uses `RelDataTypeSystemImpl` to compute the scale/precision, which behaves the same as JavaTypeFactoryImpl.
```
...
case REAL:
return 7;
case FLOAT:
case DOUBLE:
return 15;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhioncbr closed pull request #10814: [multistage][float-double-comparison]: updated for CAST function addition based on correct precision.
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr closed pull request #10814: [multistage][float-double-comparison]: updated for CAST function addition based on correct precision.
URL: https://github.com/apache/pinot/pull/10814
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhioncbr commented on pull request #10814: float-double-comparison: updated for avoiding right cast transformation incase of float…
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on PR #10814:
URL: https://github.com/apache/pinot/pull/10814#issuecomment-1573065703
Thanks for the help @Jackie-Jiang for the right pointers.
I figured out how we can handle the `CAST` function addition based on the data type precision
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on pull request #10814: [multistage][float-double-comparison]: updated for CAST function addition based on correct precision.
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on PR #10814:
URL: https://github.com/apache/pinot/pull/10814#issuecomment-1573950275
> I found that the CAST transformation will happen based on the evaluation of this [function](https://github.com/apache/calcite/blob/main/core/src/main/java/org/apache/calcite/rel/type/RelDataTypeSystemImpl.java#L62). The `CAST` transformation applies to the data type with lower precision. In the case of INT-LONG, Integer has lower precision(10) vs BigInt(19). However, In the case of FLOAT-DOUBLE, both have the same precision value and hence CAST applies to the RHS. I override the method in my latest commit by assigning `DOUBLE` more precision value than `FLOAT` to get the correct results.
hmm. this seems incorrect. according to `RelDataTypeFactoryImpl`
```
case FLOAT:
return createSqlType(SqlTypeName.DECIMAL, 14, 7);
case DOUBLE:
// the default max precision is 19, so this is actually DECIMAL(19, 15)
// but derived system can override the max precision/scale.
return createSqlType(SqlTypeName.DECIMAL, 30, 15);
```
I think there are some discrepancy when we overwrite TypeSystem vs TypeFactory
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhioncbr commented on pull request #10814: [multistage][float-double-comparison]: updated for CAST function addition based on correct precision.
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on PR #10814:
URL: https://github.com/apache/pinot/pull/10814#issuecomment-1573990610
> > I found that the CAST transformation will happen based on the evaluation of this [function](https://github.com/apache/calcite/blob/main/core/src/main/java/org/apache/calcite/rel/type/RelDataTypeSystemImpl.java#L62). The `CAST` transformation applies to the data type with lower precision. In the case of INT-LONG, Integer has lower precision(10) vs BigInt(19). However, In the case of FLOAT-DOUBLE, both have the same precision value and hence CAST applies to the RHS. I override the method in my latest commit by assigning `DOUBLE` more precision value than `FLOAT` to get the correct results.
>
> hmm. this seems incorrect.
>
> 1. according to `RelDataTypeFactoryImpl`
>
> ```
> case FLOAT:
> return createSqlType(SqlTypeName.DECIMAL, 14, 7);
> case DOUBLE:
> // the default max precision is 19, so this is actually DECIMAL(19, 15)
> // but derived system can override the max precision/scale.
> return createSqlType(SqlTypeName.DECIMAL, 30, 15);
> ```
>
> I think there are some discrepancy when we overwrite TypeSystem vs TypeFactory
>
> 2. even if the precision is the same, not considering scale as a cast reason is still incorrect IMO
Let me look into it. Thanks for the pointers.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhioncbr commented on pull request #10814: [multistage][float-double-comparison]: updated for CAST function addition based on correct precision.
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on PR #10814:
URL: https://github.com/apache/pinot/pull/10814#issuecomment-1589816555
Okay, it's good to be merged. Thanks, @walterddr, for the help.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on pull request #10814: [multistage][float-double-comparison]: updated for CAST function addition based on correct precision.
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on PR #10814:
URL: https://github.com/apache/pinot/pull/10814#issuecomment-1587731174
thanks for the contribution @abhioncbr, i will try to take another look today!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhioncbr commented on a diff in pull request #10814: [multistage][float-double-comparison]: updated for CAST function addition based on correct precision.
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on code in PR #10814:
URL: https://github.com/apache/pinot/pull/10814#discussion_r1224908703
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/type/TypeSystem.java:
##########
@@ -70,4 +70,16 @@ public RelDataType deriveAvgAggType(RelDataTypeFactory typeFactory,
}
}
}
+
+ // overriding the function for handling comparison between different data types.
+ // based on the precision value calcite adds the CAST function to make operands of same type for comparison.
+ @Override public int getDefaultPrecision(SqlTypeName typeName) {
+ switch (typeName) {
+ case DOUBLE:
+ return 16;
+ default:
+ // Following BasicSqlType precision as the default
+ return super.getDefaultPrecision(typeName);
+ }
Review Comment:
Well, I changed the `TypeFactory` extending`SqlTypeFactoryImpl` instead of `JavaTypeFactoryImpl`, and there is no effect on our test suite. Here are the [changes](https://github.com/apache/pinot/pull/10883).
I will say either our test suite isn't complete to cover complete testing or it doesn't matter if we extend either type. Let me look `TypeCoercion` logic.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhioncbr commented on pull request #10814: float-double-comparison: updated for avoiding right cast transformation incase of float…
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on PR #10814:
URL: https://github.com/apache/pinot/pull/10814#issuecomment-1569422018
> I feel this is not the right fix. The problem is that we always fill the value based on the value type of the LHS. Instead, we should use the common super type of LHS and RHS to perform the comparison. E.g. For INT - LONG comparison, we should read LONG values instead of INT regardless of which side has INT type.
Thanks for the reference.
I tried to debug the int-long comparison query and found that from calcite, we are getting cast of `Int` to `Bigint` even though the `Int` column is in LHS.
<img width="1584" alt="Screen Shot 2023-05-30 at 10 43 59 PM" src="https://github.com/apache/pinot/assets/1076944/f5376f04-d910-4258-bf69-b27444b54b94">
This is not the case with float-double comparison.
I wonder if it is happening by itself from the calcite site or if we have some logic to control the behaviour of calcite? Any pointers?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhioncbr closed pull request #10814: [multistage][float-double-comparison]: updated for CAST function addition based on correct precision.
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr closed pull request #10814: [multistage][float-double-comparison]: updated for CAST function addition based on correct precision.
URL: https://github.com/apache/pinot/pull/10814
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on pull request #10814: float-double-comparison: updated for avoiding right cast transformation incase of float…
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #10814:
URL: https://github.com/apache/pinot/pull/10814#issuecomment-1572787445
cc @walterddr ^^
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org