HyukjinKwon commented on PR #57994:
URL: https://github.com/apache/spark/pull/57994#issuecomment-5287319472
Thanks for the fix — the approach looks correct and is backward-compatible.
When validation is off, `removeLeadingZerosFromNumberTypePartition` catches
`NumberFormatException` and returns the raw value; all six numeric types
(`Byte`…`Double`) parse via
`Integer.parseInt`/`JLong.parseLong`/`JDouble.parseDouble`, which throw exactly
`java.lang.NumberFormatException`, so the `catch` matches precisely without
swallowing anything broader, and with validation on (the default) the exception
still propagates. A few non-blocking points:
**1. Loop-invariant config read** —
`InsertIntoHadoopFsRelationCommand.scala` (`getCustomPartitionLocations`):
`conf.validatePartitionColumns` is now evaluated once per partition inside
`partitions.flatMap { p => ... }`, but its value can't change during the
traversal. Each call is a `SQLConf.get` + `ConcurrentHashMap` lookup, so this
repeats per partition. Suggest hoisting it:
```scala
val validatePartitionColumns = conf.validatePartitionColumns
partitions.flatMap { p =>
val defaultLocation = qualifiedOutputPath.suffix(
"/" + PartitioningUtils.getPathFragment(
p.spec, table.partitionSchema, validatePartitionColumns)).toString
...
}
```
(The other new call site, in the static-partition branch, is a single call
and is fine.)
**2. Test coverage** — the new tests exercise
`PartitioningUtils.getPathFragment` directly, which is good, but not the
end-to-end `InsertIntoHadoopFsRelationCommand` insert/refresh path that
motivated the ticket. An integration test (INSERT into a partitioned table
whose on-disk partition value is a non-numeric string, with
`spark.sql.sources.validatePartitionColumns=false`) would guard the actual
scenario.
**3. Pre-existing limitation (not introduced here, just flagging)** — the
fix rescues values that fail numeric parsing, but a numeric-looking value with
leading zeros (e.g. `007`) is still normalized to `7` regardless of the flag,
so a legacy directory like `p_int=007` still wouldn't be matched. Worth
confirming that isn't part of the reported scenario.
--
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]
