You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by "thomasrebele (via GitHub)" <gi...@apache.org> on 2026/07/09 15:45:03 UTC
Re: [PR] HIVE-29641: Upgrade Calcite to 1.42.0 [hive]
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org