viirya commented on code in PR #57248:
URL: https://github.com/apache/spark/pull/57248#discussion_r3581306974
##########
sql/core/src/test/scala/org/apache/spark/sql/connector/KeyGroupedPartitioningSuite.scala:
##########
@@ -4469,4 +4469,34 @@ class KeyGroupedPartitioningSuite extends
DistributionAndOrderingSuiteBase with
}
}
}
+
+ test("SPARK-58120: doCanonicalize preserves keyGroupedPartitioning
expression order") {
+ // Regression test: BatchScanExec.doCanonicalize previously used
+ // QueryPlan.normalizePredicates which combines expressions with And,
canonicalizes,
+ // then splits back. This reorders expressions, causing a mismatch between
+ // expression data types and partition key row values, leading to
ClassCastException.
+ val partition = Array(identity("id"), identity("data"))
+ createTable(table, columns, partition)
+ sql(s"INSERT INTO testcat.ns.$table VALUES " +
+ "(1, 'aa', cast('2020-01-01' as timestamp)), " +
+ "(2, 'bb', cast('2021-01-01' as timestamp)), " +
+ "(3, 'cc', cast('2022-01-01' as timestamp))")
+
+ val df = sql(s"SELECT id, data FROM testcat.ns.$table")
+ val scan = df.queryExecution.executedPlan.collect { case b: BatchScanExec
=> b }.head
+ val canonicalized = scan.canonicalized.asInstanceOf[BatchScanExec]
+
+ // The canonicalized keyGroupedPartitioning expressions must preserve the
same order
+ // as the original: [id (IntegerType), data (StringType)], not reversed.
+ val originalTypes = scan.keyGroupedPartitioning.get.map(_.dataType)
+ val canonicalizedTypes =
canonicalized.keyGroupedPartitioning.get.map(_.dataType)
+ assert(originalTypes == canonicalizedTypes,
Review Comment:
The fix looks right, and asserting on the canonicalized dataType order is
the correct observable for this bug. One question on the test's discriminating
power: whether the old code actually reorders this particular pair depends on
the `hashCode` ordering of the two canonicalized attributes (`orderCommutative`
sorts `And` operands by hash), which is deterministic but essentially arbitrary
per (name, type, exprId) combination. If `[id, data]` happened to sort in the
original order, this test would pass even without the fix and wouldn't guard
against a reintroduction. Could you confirm the test fails on master without
the fix? If it does (which the JIRA repro suggests), all good - might be worth
a brief note in the test comment that the id/data pair is known to reorder
under hashCode, so a future refactor of the test columns doesn't silently lose
the coverage.
--
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]
