zabetak commented on code in PR #6523:
URL: https://github.com/apache/hive/pull/6523#discussion_r3595228728
##########
iceberg/iceberg-handler/src/test/results/positive/dynamic_partition_pruning.q.out:
##########
@@ -1458,37 +1458,40 @@ STAGE PLANS:
Map Operator Tree:
TableScan
alias: srcpart_double_hour_n0
- filterExpr: ((UDFToDouble(hour) = 11.0D) and CAST(
UDFToInteger((hr / 2.0D)) AS STRING) is not null) (type: boolean)
+ filterExpr: ((UDFToDouble(hour) = 11.0D) and hr is not null)
(type: boolean)
Statistics: Num rows: 2 Data size: 188 Basic stats: COMPLETE
Column stats: COMPLETE
Filter Operator
- predicate: ((UDFToDouble(hour) = 11.0D) and CAST(
UDFToInteger((hr / 2.0D)) AS STRING) is not null) (type: boolean)
+ predicate: ((UDFToDouble(hour) = 11.0D) and hr is not
null) (type: boolean)
Review Comment:
The new plan is equivalent but it seems that the simplification didn't lead
to a significant improvement. The simplification here seems to have introduced
a new `Filter[col0 is not null]` higher up so at the end of the day the number
of comparisons remains the same.
##########
ql/src/test/org/apache/hadoop/hive/ql/optimizer/calcite/TestHiveRelJsonReader.java:
##########
@@ -57,6 +58,8 @@ static Stream<Path> inputJsonFiles() throws IOException {
@ParameterizedTest
@MethodSource("inputJsonFiles")
void testReadJson(Path jsonFile) throws IOException {
+ Hive.setCalciteSystemProperties();
+
Review Comment:
Too intrusive; let's find another way to set these.
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/views/HiveMaterializedViewRule.java:
##########
@@ -145,31 +153,142 @@ public HiveMaterializedViewOnlyAggregateRule(
super(relBuilderFactory, generateUnionRewriting,
unionRewritingPullProgram);
}
+ // Overridden to avoid CALCITE-7641; TODO remove this once fixed.
+ @Override protected @Nullable RelNode rewriteQuery(
+ RelBuilder relBuilder,
+ RexBuilder rexBuilder,
+ RexSimplify simplify,
+ RelMetadataQuery mq,
+ RexNode compensationColumnsEquiPred,
+ RexNode otherCompensationPred,
+ @Nullable Project topProject,
+ RelNode node,
+ BiMap<RexTableInputRef.RelTableRef, RexTableInputRef.RelTableRef>
queryToViewTableMapping,
+ EquivalenceClasses viewEC, EquivalenceClasses queryEC) {
+ if (Bug.CALCITE_7641_FIXED) {
+ throw new AssertionError("Overriding should be removed");
+ }
+ RelNode unwrappedNode = HiveCalciteUtil.stripHepVertices(node);
+ return super.rewriteQuery(relBuilder, rexBuilder, simplify, mq,
compensationColumnsEquiPred,
+ otherCompensationPred, topProject, unwrappedNode,
queryToViewTableMapping, viewEC, queryEC);
+ }
+
@Override
protected SqlFunction getFloorSqlFunction(TimeUnitRange flag) {
return HiveRelBuilder.getFloorSqlFunction(flag);
}
}
- /**
- * This rule is used within the PROGRAM that rewrites the query for
- * partial rewritings. Its goal is to extract the RelNode from the
- * HepRelVertex node so the rest of the rules in the PROGRAM can be
- * applied correctly.
- */
- private static class HiveHepExtractRelNodeRule extends RelOptRule {
+ protected static class HiveMaterializedViewProjectFilterRule extends
MaterializedViewProjectFilterRule {
+ public HiveMaterializedViewProjectFilterRule(RelBuilderFactory
relBuilderFactory,
+ boolean generateUnionRewriting, HepProgram unionRewritingPullProgram,
+ boolean fastBailOut) {
+ super(relBuilderFactory, generateUnionRewriting,
unionRewritingPullProgram, fastBailOut);
+ }
+
+ // Overridden to avoid CALCITE-7641; TODO remove this once fixed.
+ @Override protected @Nullable RelNode rewriteQuery(
+ RelBuilder relBuilder,
+ RexBuilder rexBuilder,
+ RexSimplify simplify,
+ RelMetadataQuery mq,
+ RexNode compensationColumnsEquiPred,
+ RexNode otherCompensationPred,
+ @Nullable Project topProject,
+ RelNode node,
+ BiMap<RexTableInputRef.RelTableRef, RexTableInputRef.RelTableRef>
viewToQueryTableMapping,
+ EquivalenceClasses viewEC, EquivalenceClasses queryEC) {
+ if (Bug.CALCITE_7641_FIXED) {
+ throw new AssertionError("Overriding should be removed");
+ }
+ RelNode unwrappedNode = HiveCalciteUtil.stripHepVertices(node);
+ return super.rewriteQuery(relBuilder, rexBuilder, simplify, mq,
compensationColumnsEquiPred,
+ otherCompensationPred, topProject, unwrappedNode,
viewToQueryTableMapping, viewEC, queryEC);
+ }
+ }
+
+ protected static class HiveMaterializedViewOnlyFilterRule extends
MaterializedViewOnlyFilterRule {
+ public HiveMaterializedViewOnlyFilterRule(RelBuilderFactory
relBuilderFactory,
+ boolean generateUnionRewriting, HepProgram unionRewritingPullProgram,
+ boolean fastBailOut) {
+ super(relBuilderFactory, generateUnionRewriting,
unionRewritingPullProgram, fastBailOut);
+ }
- private static final HiveHepExtractRelNodeRule INSTANCE =
- new HiveHepExtractRelNodeRule();
+ // Overridden to avoid CALCITE-7641; TODO remove this once fixed.
+ @Override protected @Nullable RelNode rewriteQuery(
+ RelBuilder relBuilder,
+ RexBuilder rexBuilder,
+ RexSimplify simplify,
+ RelMetadataQuery mq,
+ RexNode compensationColumnsEquiPred,
+ RexNode otherCompensationPred,
+ @Nullable Project topProject,
+ RelNode node,
+ BiMap<RexTableInputRef.RelTableRef, RexTableInputRef.RelTableRef>
viewToQueryTableMapping,
+ EquivalenceClasses viewEC, EquivalenceClasses queryEC) {
+ if (Bug.CALCITE_7641_FIXED) {
+ throw new AssertionError("Overriding should be removed");
+ }
+ RelNode unwrappedNode = HiveCalciteUtil.stripHepVertices(node);
+ return super.rewriteQuery(relBuilder, rexBuilder, simplify, mq,
compensationColumnsEquiPred,
+ otherCompensationPred, topProject, unwrappedNode,
viewToQueryTableMapping, viewEC, queryEC);
+ }
+ }
- private HiveHepExtractRelNodeRule() {
- super(operand(HepRelVertex.class, any()));
+ protected static class HiveMaterializedViewProjectJoinRule extends
MaterializedViewProjectJoinRule {
+ public HiveMaterializedViewProjectJoinRule(RelBuilderFactory
relBuilderFactory,
+ boolean generateUnionRewriting, HepProgram unionRewritingPullProgram,
+ boolean fastBailOut) {
+ super(relBuilderFactory, generateUnionRewriting,
unionRewritingPullProgram, fastBailOut);
}
- @Override
- public void onMatch(RelOptRuleCall call) {
- final HepRelVertex rel = call.rel(0);
- call.transformTo(rel.getCurrentRel());
+ // Overridden to avoid CALCITE-7641; TODO remove this once fixed.
+ @Override protected @Nullable RelNode rewriteQuery(
+ RelBuilder relBuilder,
+ RexBuilder rexBuilder,
+ RexSimplify simplify,
+ RelMetadataQuery mq,
+ RexNode compensationColumnsEquiPred,
+ RexNode otherCompensationPred,
+ @Nullable Project topProject,
+ RelNode node,
+ BiMap<RexTableInputRef.RelTableRef, RexTableInputRef.RelTableRef>
viewToQueryTableMapping,
+ EquivalenceClasses viewEC, EquivalenceClasses queryEC) {
+ if (Bug.CALCITE_7641_FIXED) {
+ throw new AssertionError("Overriding should be removed");
+ }
+ RelNode unwrappedNode = HiveCalciteUtil.stripHepVertices(node);
+ return super.rewriteQuery(relBuilder, rexBuilder, simplify, mq,
compensationColumnsEquiPred,
+ otherCompensationPred, topProject, unwrappedNode,
viewToQueryTableMapping, viewEC, queryEC);
+ }
+ }
+
+ protected static class HiveMaterializedViewOnlyJoinRule extends
MaterializedViewOnlyJoinRule {
+ public HiveMaterializedViewOnlyJoinRule(RelBuilderFactory
relBuilderFactory,
+ boolean generateUnionRewriting, HepProgram unionRewritingPullProgram,
+ boolean fastBailOut) {
+ super(relBuilderFactory, generateUnionRewriting,
unionRewritingPullProgram, fastBailOut);
+ }
+
+ // Overridden to avoid CALCITE-7641; TODO remove this once fixed.
Review Comment:
nit: We can drop the TODO comments since we already have the `Bug` reference
in the method. By doing this I guess we can get rid of some Sonar warnings.
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/views/HiveMaterializedViewRule.java:
##########
@@ -78,36 +86,16 @@ public class HiveMaterializedViewRule {
.build();
public static final MaterializedViewProjectFilterRule
INSTANCE_PROJECT_FILTER =
- (MaterializedViewProjectFilterRule)
MaterializedViewProjectFilterRule.Config.DEFAULT
- .withGenerateUnionRewriting(true)
- .withFastBailOut(false)
- .withUnionRewritingPullProgram(PROGRAM)
- .withRelBuilderFactory(HiveRelFactories.HIVE_BUILDER)
- .toRule();
+ new HiveMaterializedViewProjectFilterRule(HiveRelFactories.HIVE_BUILDER,
true, PROGRAM, false);
Review Comment:
nit: we could simplify the constructor to `new
HiveMaterializedViewProjectFilterRule()` since we are always passing in the
same param values.
##########
druid-handler/pom.xml:
##########
@@ -353,10 +353,24 @@
<pattern>io.netty</pattern>
<shadedPattern>org.apache.hive.druid.io.netty</shadedPattern>
</relocation>
+ <!--
+ Calcite is intentionally NOT included or relocated here.
+ Calcite's calcite-core ships
org/apache/calcite/runtime/SqlFunctions.class with a
+ malformed RuntimeVisibleTypeAnnotations attribute
(LOCAL_VARIABLE start_pc/length
+ pointing inside instruction operands in the static
initializer). The JVM tolerates it;
+ ASM's ClassRemapper does not, and shading + javadoc fails
with
+ "Index 65536 out of bounds for length 334 [...] Error in
ASM processing class".
+ Present in Calcite 1.35.0 through at least 1.42.0.
Upstream: ASM gitlab issue
+ https://gitlab.ow2.org/asm/asm/-/issues/318008 (closed as
invalid: root cause is on
+ Calcite's side); Calcite ticket:
https://issues.apache.org/jira/browse/CALCITE-6393
+ (still unresolved). Druid 0.17.1 uses Calcite APIs
compatible with Hive's Calcite 1.42+,
+ so skipping this relocation is safe. If Druid is upgraded
to an incompatible Calcite,
+ restore the relocation (either after Calcite fixes their
bytecode or by adding a shade
+ filter that excludes SqlFunctions.class from remapping).
Review Comment:
Relocation is one thing and inclusion in the shaded jar is another. I
suppose they included calcite in the shaded jar because the latter is send to
workers (running in different JVMs) with a different classpath. Not sure if the
Druid module can still work if you remove calcite from shading.
Druid 0.17.1 is using calcite 1.21.0. I highly doubt that all the used APIs
are compatible with 1.42.0. In fact, I am rather skeptical about the
compatibility with previous upgrades as well (1.25.0 and 1.33.0).
Out of curiosity, I tried running `TestMiniDruidCliDriver` today and
realized that the Druid module is broken since 2021. I send also an email to
https://lists.apache.org/thread/qr0xxkvvg4wrlz4kg0bmktldw96yvpn2 explaining the
current status and favoring the complete removal of the module.
All in all, the changes here do not really matter and we don't have to sweat
too much about it since it's only a matter of days before getting rid of the
entire module.
##########
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:
I am in favor of keeping the existing behavior via overriding for now.
However, it makes me wonder when and why we are using the Calcite derivation
logic. Hive has its own type derivation rules for division which are applied
via
`org.apache.hadoop.hive.ql.udf.generic.GenericUDFOPDivide#deriveResultDecimalTypeInfo`.
I get the feeling that this method should use the logic in
`GenericUDFOPDivide`. This is another important follow-up I guess.
##########
ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java:
##########
@@ -559,6 +560,14 @@ public static void closeCurrent() {
hiveDB.remove();
}
+ public static void setCalciteSystemProperties() {
+ // 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:
Setting calcite properties at this stage is very brittle. The
`CalciteSystemProperty` class is initialized during class loading and remains
immutable afterwards. This means that if during the JVM startup we load
`CalciteSystemProperty.class` before `Hive.class` this settings will never take
effect.
I know that this pattern was used before and it may have been my idea to do
it as such in the first place but its definitely something that we should fix.
I guess a better alternative would be to put these settings in
`saffron.properties` file and package this inside the Hive jar (e.g., under
src/main/resources).
This is not a blocker but something that we definitely need to follow-up if
not treated here.
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/reloperators/HiveAggregate.java:
##########
@@ -75,8 +75,8 @@ private static AggregateCall replaceAggFunction(RelDataType
rowType, AggregateCa
SqlTypeUtil.projectTypes(rowType, aggCall.getArgList()),
aggCall.getType());
return AggregateCall.create(aggFunction, aggCall.isDistinct(),
aggCall.isApproximate(), aggCall.ignoreNulls(),
- aggCall.getArgList(), aggCall.filterArg, aggCall.distinctKeys,
aggCall.getCollation(), aggCall.getType(),
- aggCall.getName());
+ aggCall.rexList, aggCall.getArgList(), aggCall.filterArg,
aggCall.distinctKeys, aggCall.getCollation(),
+ aggCall.getType(), aggCall.getName());
Review Comment:
Logged https://issues.apache.org/jira/browse/CALCITE-7659 for having a more
convenient way to do this copy.
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveRelFieldTrimmer.java:
##########
@@ -517,7 +517,8 @@ private ImmutableBitSet generateNewGroupset(Aggregate
aggregate, ImmutableBitSet
RelMetadataQuery mq = aggregate.getCluster().getMetadataQuery();
final Set<ImmutableBitSet> uniqueKeys = mq.getUniqueKeys(input, false);
- if (uniqueKeys == null || uniqueKeys.isEmpty()) {
+ if (uniqueKeys == null || uniqueKeys.isEmpty() ||
+ (uniqueKeys.size() == 1 && uniqueKeys.iterator().next().isEmpty())) {
Review Comment:
If uniqueKeys is the empty set (`{}`) then all columns are considered unique
according to the Javadoc. I think we can avoid calling
`generateGroupSetIfCardinalitySame` which can be expensive (since it involves
metadata calls to lineage) and handle the case either in the for-loop below
either in a separate if-block.
Basically when unique keys is the empty set I guess we can drop all unused
columns as far as we keep at least one in place to avoid altering the group by
semantics and hitting the bug you outlined in `empty_result.q` file.
##########
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)
- || (aggregate.getGroupSet().isEmpty())
- || fieldsUsed.contains(aggregate.getGroupSet())) {
+ if (aggregate.getGroupSet().isEmpty()
+ || fieldsUsed.intersects(aggregate.getGroupSet())) {
Review Comment:
Indeed the new condition seems more aligned with the Javadoc. However, it
seems that there is at least one more bug in this method since I don't see any
handling of grouping sets. It's not related to the upgrade so let's create a
follow-up to fix it later.
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/type/HiveFunctionHelper.java:
##########
@@ -336,6 +336,19 @@ public RexNode getExpression(String functionText,
FunctionInfo fi,
RexUtil.flatten(call.getOperands(), call.getOperator()));
}
+ // Avoid creating incorrect expressions like $1 < NULL or $1 = NULL or
NULL = NULL
+ // which may be problematic for Calcite later on
+ if (expr.isA(SqlKind.BINARY_COMPARISON) && expr.getKind() !=
SqlKind.IS_DISTINCT_FROM
+ && expr.getKind() != SqlKind.IS_NOT_DISTINCT_FROM) {
+ RexCall call = (RexCall) expr;
+ RexNode op0 = call.getOperands().get(0);
+ RexNode op1 = call.getOperands().get(1);
+ if ((op0.getKind() == SqlKind.LITERAL && ((RexLiteral) op0).isNull()) ||
+ (op1.getKind() == SqlKind.LITERAL && ((RexLiteral) op1).isNull())) {
+ expr = rexBuilder.makeNullLiteral(expr.getType());
+ }
+ }
+
Review Comment:
Can we replace this code with a call to
`RexSimplify#simplifyComparisonWithNull`?
The fact that we have to call simplification code here in order to avoid an
`AssertionError` later on is worrisome. The choice to throw `AssertionError`
from core calcite APIs was somewhat brutal. If we fail to handle "invalid"
comparisons on some other part of the code we may end-up with irrecoverable
errors at runtime. Anyways let's hope that its sufficient to update the code
here and get done with it.
##########
ql/pom.xml:
##########
@@ -365,6 +365,24 @@
<artifactId>hadoop-yarn-client</artifactId>
<optional>true</optional>
</dependency>
+ <!--
+ Runtime dependencies required by
org.apache.calcite.avatica:avatica-core:1.28.0
+ (pulled in by Calcite 1.42+): Avatica now uses Apache HttpComponents 5
for its
+ HTTP/JDBC transport, so hive-exec must bundle httpcore5/httpclient5 or
tests fail
+ with "NoClassDefFoundError: org/apache/hc/core5/util/Timeout". Avatica
declares
+ these as <scope>runtime</scope> transitively, but they must be declared
here
+ explicitly because Hive's enforcer bans org.apache.hc.core5/client5.**
imports
+ elsewhere and the hive-exec shade plugin only bundles ql-level
dependencies.
+ Can be removed once Hive migrates to HttpComponents 5 across the board.
+ -->
+ <dependency>
+ <groupId>org.apache.httpcomponents.core5</groupId>
+ <artifactId>httpcore5</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.httpcomponents.client5</groupId>
+ <artifactId>httpclient5</artifactId>
+ </dependency>
Review Comment:
First of all its unfortunate that we inherited yet another dependency from
Avatica that we need to bundle in hive-exec but we don't have much of a choice
right now. We could try to get rid of it in a subsequent Avatica release by
repackaging the client differently.
Secondly, we may need to include the httpcore5 in the shaded jar otherwise
queries may break if the dependencies are required at query runtime. Query
execution happens in Tez and in a real cluster this has a different classpath
than HS2. The shaded jar is part of the Tez classpath but not the HS2/ql
dependencies. Tests cannot capture this kind of behavior so to test if
everything works fine the easier would be to build the Hive docker image in the
packaging module, run it, and test some DDL, DML, SQL, operations to see if
everything works fine.
Thirdly, the statement that shade plugin bundles only ql-level dependencies
is not true. You can pick which transitive dependencies are included by the
appropriate `include` statements. The most vivid example is Avatica; it is not
direct dependency of the ql module but it is included in the shaded jar.
Finally, if we end-up declaring httpcore5 explicitly here then probably we
have to use `runtime` scope if we don't want to accidentally start using it the
code.
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveRelFieldTrimmer.java:
##########
@@ -517,7 +517,8 @@ private ImmutableBitSet generateNewGroupset(Aggregate
aggregate, ImmutableBitSet
RelMetadataQuery mq = aggregate.getCluster().getMetadataQuery();
final Set<ImmutableBitSet> uniqueKeys = mq.getUniqueKeys(input, false);
- if (uniqueKeys == null || uniqueKeys.isEmpty()) {
+ if (uniqueKeys == null || uniqueKeys.isEmpty() ||
+ (uniqueKeys.size() == 1 && uniqueKeys.iterator().next().isEmpty())) {
Review Comment:
Apart from that it's worth double-checking other calls of `getUniqueKeys` in
Hive code base (e.g., `HiveAggregateJoinTransposeRule`) to ensure that we are
not going to introduce wrong results due to the new behavior.
--
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]
