pratham76 commented on PR #57994:
URL: https://github.com/apache/spark/pull/57994#issuecomment-5301042949
> 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:
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.)
Thanks for pointing it out, have addressed.
> 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.
Have refactored the tests to include the actual scenario of `INSERT into a
partitioned table whose on-disk partition value is a non-numeric string, with
spark.sql.sources.validatePartitionColumns=false`
> 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.
I believe normalization should happen for parseable values. Since "007" can
be successfully parsed as an integer, it should be normalized to "7" for
semantic correctness, both represent the same numeric value. The
`validatePartitionColumns=false` flag is intended to tolerate type mismatches
(e.g., non-numeric strings like "partition_value" in an IntegerType column),
not to disable all type-aware processing. Normalizing parseable numeric values
maintains consistency with how Spark handles numeric partition values. I have
added a comment notifying about this behaviour. Please do provide your thoughts
on this.. Thanks!
--
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]
