mbutrovich commented on code in PR #4752:
URL: https://github.com/apache/datafusion-comet/pull/4752#discussion_r3691297130
##########
spark/src/main/scala/org/apache/comet/serde/operator/CometIcebergNativeScan.scala:
##########
@@ -343,15 +357,65 @@ object CometIcebergNativeScan extends
CometOperatorSerde[CometBatchScanExec] wit
fileScanTaskClass: Class[_],
taskBuilder: OperatorOuterClass.IcebergFileScanTask.Builder,
commonBuilder: OperatorOuterClass.IcebergScanCommon.Builder,
- partitionTypeToPoolIndex: mutable.HashMap[String, Int],
partitionSpecToPoolIndex: mutable.HashMap[String, Int],
partitionDataToPoolIndex: mutable.HashMap[String, Int]): Unit = {
try {
val specMethod = fileScanTaskClass.getMethod("spec")
val spec = specMethod.invoke(task)
if (spec != null) {
- // Deduplicate partition spec
+ // Get the partition type/schema from the spec. Needed regardless of
whether this task
Review Comment:
Raising this again from my first pass, since we're already asking for
another round of changes.
`spark/src/main/scala/org/apache/comet/serde/operator/CometIcebergNativeScan.scala`,
`serializePartitionData` (currently lines 354-533):
- Lines 419-444 compute the real spec's JSON via reflection
(`PartitionSpecParser.toJson`), dedup it into `partitionSpecToPoolIndex`, add
the paired `partitionTypeJson` to the pool, and call
`taskBuilder.setPartitionSpecIdx(specIdx)`. This runs unconditionally, before
we know whether the task has partition values.
- Lines 496-513 then check `partitionValues.isEmpty`, and if it's true,
compute `specId` via another reflection call, build a separate `emptySpecJson`,
dedup that into the same map under a different key, and overwrite
`taskBuilder.setPartitionSpecIdx` with the empty-spec index. The real spec
entry added at line 429-443 is still sitting in `partitionSpecToPoolIndex` and
`commonBuilder`'s pools; it's just not referenced by this task anymore.
So every value-less task (partition evolution: a file written after a
partition field was dropped) still does the toJson reflection call, the JSON
string dedup lookup, and interns an unused pool entry, only to throw the result
away a few lines later. The fix from my earlier comment stands: check
`partitionValues.isEmpty` (or rather, whether `partitionData` has any
non-unknown fields) before running the lines 419-444 block, so the value-less
path only ever computes `emptySpecJson`, not both.
The `fieldsJson` computation right above this (lines 367-412) is fine to
leave unconditional since its own comment says it's needed regardless of the
value-less path -- it feeds `partitionValues` either way. It's specifically the
spec-serialization-and-pool-insert block at 419-444 that should move after the
value-less check, not before it.
--
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]
