thomasrebele commented on code in PR #6523:
URL: https://github.com/apache/hive/pull/6523#discussion_r3550776119
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveRelFieldTrimmer.java:
##########
@@ -549,9 +550,8 @@ private ImmutableBitSet generateNewGroupset(Aggregate
aggregate, ImmutableBitSet
*/
private Aggregate rewriteGBConstantKeys(Aggregate aggregate, ImmutableBitSet
fieldsUsed,
ImmutableBitSet aggCallFields) {
- if ((aggregate.getIndicatorCount() > 0)
Review Comment:
I've seen a few other uses of getIndicatorCount(). How about creating a
follow-up ticket "Avoid deprecated Calcite code" (similar to
[HIVE-9867](https://issues.apache.org/jira/browse/HIVE-9867))?
##########
ql/pom.xml:
##########
@@ -365,6 +365,14 @@
<artifactId>hadoop-yarn-client</artifactId>
<optional>true</optional>
</dependency>
+ <dependency>
+ <groupId>org.apache.httpcomponents.core5</groupId>
+ <artifactId>httpcore5</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.httpcomponents.client5</groupId>
+ <artifactId>httpclient5</artifactId>
+ </dependency>
Review Comment:
Is there a risk of hitting other errors because of missing dependencies?
Avatica defines some more dependencies.
```
| +--- org.apache.calcite.avatica:avatica-core -> 1.28.0
| | +--- org.apache.calcite.avatica:avatica-metrics:1.28.0
| | | \--- org.slf4j:slf4j-api:1.7.25
| | +--- com.fasterxml.jackson.core:jackson-annotations:2.18.6
| | | \--- com.fasterxml.jackson:jackson-bom:2.18.6 (*)
| | +--- com.fasterxml.jackson.core:jackson-databind:2.18.6
| | | +--- com.fasterxml.jackson.core:jackson-annotations:2.18.6 (*)
| | | +--- com.fasterxml.jackson.core:jackson-core:2.18.6
| | | | \--- com.fasterxml.jackson:jackson-bom:2.18.6 (*)
| | | \--- com.fasterxml.jackson:jackson-bom:2.18.6 (*)
| | +--- com.google.protobuf:protobuf-java:3.25.8
| | +--- org.jooq:joou-java-6:0.9.4 -> 0.9.5
| | +--- com.fasterxml.jackson.core:jackson-core:2.18.6 (*)
| | +--- org.apache.httpcomponents.client5:httpclient5:5.5
| | | +--- org.apache.httpcomponents.core5:httpcore5:5.3.4 -> 5.3.5
| | | +--- org.apache.httpcomponents.core5:httpcore5-h2:5.3.4
| | | | \--- org.apache.httpcomponents.core5:httpcore5:5.3.4 ->
5.3.5
| | | \--- org.slf4j:slf4j-api:1.7.36 -> 1.7.25
| | +--- org.apache.httpcomponents.core5:httpcore5:5.3.5
| | \--- org.slf4j:slf4j-api:1.7.36 -> 1.7.25
```
Hive's pom.xml defines different version for protobuf (3.25.5),
httpcomponents (4.5.13), slf4j (1.7.30). Not sure whether this causes problems
at some point.
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveTypeSystemImpl.java:
##########
@@ -189,4 +181,39 @@ public RelDataType deriveSumType(RelDataTypeFactory
typeFactory,
return argumentType;
}
+ /**
+ * Overridden because CALCITE-6464 changed the default behavior to match
MS-SQL-Server-style algorithm,
+ * which can cause a drop in the scale computation, hence a precision loss
in certain cases.
+ * We override this method to keep the "old behavior" (pre-CALCITE-6464); an
alternative could be not
+ * overridding it (and keep the new Calcite default MS-SQL-style
decimal-divide semantics), but that
+ * would lead to "regressions" (precision loss) and would require test
adjustments.
+ */
+ @Override
+ public RelDataType deriveDecimalDivideType(RelDataTypeFactory typeFactory,
+ RelDataType type1, RelDataType type2) {
+ if (SqlTypeUtil.isExactNumeric(type1) && SqlTypeUtil.isExactNumeric(type2)
&&
+ (SqlTypeUtil.isDecimal(type1) || SqlTypeUtil.isDecimal(type2))) {
+ // Java numeric will always have invalid precision/scale,
+ // use its default decimal precision/scale instead.
+ type1 = RelDataTypeFactoryImpl.isJavaType(type1) ?
typeFactory.decimalOf(type1) : type1;
+ type2 = RelDataTypeFactoryImpl.isJavaType(type2) ?
typeFactory.decimalOf(type2) : type2;
+ int p1 = type1.getPrecision();
+ int p2 = type2.getPrecision();
+ int s1 = type1.getScale();
+ int s2 = type2.getScale();
+
+ final int maxNumericPrecision = getMaxNumericPrecision();
+ int dout = Math.min(p1 - s1 + s2, maxNumericPrecision);
+ int scale = Math.max(6, s1 + p2 + 1);
+ scale = Math.min(scale, maxNumericPrecision - dout);
+ scale = Math.min(scale, getMaxNumericScale());
+
+ int precision = dout + scale;
+ assert precision <= maxNumericPrecision;
+ assert precision > 0;
+ return typeFactory.createSqlType(SqlTypeName.DECIMAL, precision, scale);
+ }
+ return null;
+ }
+
Review Comment:
The original is [RelDataTypeSystem#L291 from Calcite
1.33](https://github.com/apache/calcite/blob/96b05ee12f936ed057265072ff6a2de8ea0a249e/core/src/main/java/org/apache/calcite/rel/type/RelDataTypeSystem.java#L291).
Code looks identical.
##########
ql/pom.xml:
##########
@@ -365,6 +365,14 @@
<artifactId>hadoop-yarn-client</artifactId>
<optional>true</optional>
</dependency>
+ <dependency>
+ <groupId>org.apache.httpcomponents.core5</groupId>
+ <artifactId>httpcore5</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.httpcomponents.client5</groupId>
+ <artifactId>httpclient5</artifactId>
+ </dependency>
Review Comment:
Also: it might be helpful to add a comment why these dependencies have been
introduced.
##########
ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java:
##########
@@ -569,6 +570,9 @@ private Hive(HiveConf c, boolean doRegisterAllFns) throws
HiveException {
conf = c;
// turn off calcite rexnode normalization
System.setProperty("calcite.enable.rexnode.digest.normalize", "false");
+ // update calcite default charset, consistent with
HiveTypeFactory#getDefaultCharset
+ System.setProperty("calcite.default.charset",
ConversionUtil.NATIVE_UTF16_CHARSET_NAME);
+ System.setProperty("calcite.default.nationalcharset",
ConversionUtil.NATIVE_UTF16_CHARSET_NAME);
Review Comment:
How about a static helper function which sets these properties, and is
called from TestHiveRelJsonReader as well?
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRelBuilder.java:
##########
@@ -89,7 +89,8 @@ public static RelBuilderFactory proto(final Context context) {
return new RelBuilderFactory() {
@Override
public RelBuilder create(RelOptCluster cluster, RelOptSchema schema) {
- Context confContext =
Contexts.of(Config.DEFAULT.withPruneInputOfAggregate(Bug.CALCITE_4513_FIXED));
+ Context confContext =
Contexts.of(Config.DEFAULT.withPruneInputOfAggregate(Bug.CALCITE_4513_FIXED)
+ .withSimplifyValues(false)); // disabled to avoid simplifications
that can create non-empty HiveValues
Review Comment:
Some context: Somehow non-empty HiveValues are not supported by the
ASTConverter, see https://github.com/apache/hive/pull/3588/changes#r983490937.
##########
druid-handler/pom.xml:
##########
@@ -353,10 +353,15 @@
<pattern>io.netty</pattern>
<shadedPattern>org.apache.hive.druid.io.netty</shadedPattern>
</relocation>
+ <!-- Calcite is intentionally NOT included or relocated
here. Druid 0.17.1 uses Calcite APIs
+ that are compatible with Hive's Calcite 1.42+. Including
calcite-core in the shade caused
+ SqlFunctions.class to exceed the JVM 64KB method limit
after relocation. If Druid is upgraded
+ to a version with an incompatible Calcite, this relocation
must be restored (excluding SqlFunctions,
+ or splitting the class via a source-level patch to
Calcite).
Review Comment:
I had a look at SqlFunctions.class (in
`~/.m2/repository/org/apache/calcite/calcite-core/1.42.0/calcite-core-1.42.0-sources.jar!/org/apache/calcite/runtime/SqlFunctions.java`),
but I don't understand how it hits the 64KB method limit after shading. How to
reproduce the problem with the limit? I've reverted the changes to the file,
and run the build (`mvn clean install -DskipTests -Pitests,dist
-Denforcer.skip=true -T1C`) and the druid-handler tests (`mvn test -pl
druid-handler`) successfully.
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/views/HiveAugmentSnapshotMaterializationRule.java:
##########
@@ -146,6 +146,12 @@ public void onMatch(RelOptRuleCall call) {
final RelBuilder relBuilder = call.builder();
relBuilder.push(tableScan);
+ if (snapshotId == null) {
+ // Avoid creating an incorrect expression $snapshotIdInputRef <= NULL
+ // which may be problematic for Calcite later on; instead use a special
value -1,
+ // which will be later interpreted by HivePushdownSnapshotFilterRule
(and removed)
+ snapshotId = -1L;
+ }
Review Comment:
Iceberg snapshot IDs are positive, and -1 may be used to indicate null
values (see
[spec](https://iceberg.apache.org/spec/?h=snapshot+id#assignment-of-snapshot-ids-and-current-snapshot-id)).
I don't know whether non-null snapshotId's are positive for non-iceberg
tables. @kasakrisz, can you confirm? Is there any risk of numeric overflow?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
