hudi-agent commented on code in PR #19703:
URL: https://github.com/apache/hudi/pull/19703#discussion_r3832120198
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/spark/sql/hudi/HoodieSqlCommonUtils.scala:
##########
@@ -406,24 +406,55 @@ object HoodieSqlCommonUtils extends SparkAdapterSupport {
private def makePartitionPath(partitionFields: Seq[String],
normalizedSpecs: Map[String, String],
enableEncodeUrl: Boolean,
- enableHiveStylePartitioning: Boolean): String
= {
+ enableHiveStylePartitioning: Boolean,
+ slashSeparatedDatePartitioning: Boolean):
String = {
+ // NOTE: Slash-separated date partitioning only kicks in for a table
partitioned by a single
+ // (date) column, mirroring the guard in
[[KeyGenUtils#getRecordPartitionPath]] that drives
+ // the write path -- these commands have to name the very same
directory the writer created.
+ // Hive-style partitioning is excluded because the config documents
the two as mutually
+ // exclusive, and the write paths do not agree on what the
combination should produce
+ // (tracked in HUDI issue #19669), so there is no single directory
to name here
+ val slashSeparateDates =
+ slashSeparatedDatePartitioning && !enableHiveStylePartitioning &&
partitionFields.length == 1
partitionFields.map { partitionColumn =>
val encodedPartitionValue = if (enableEncodeUrl) {
PartitionPathEncodeUtils.escapePathName(normalizedSpecs(partitionColumn))
} else {
normalizedSpecs(partitionColumn)
}
- if (enableHiveStylePartitioning)
s"$partitionColumn=$encodedPartitionValue" else encodedPartitionValue
+ if (enableHiveStylePartitioning) {
+ s"$partitionColumn=$encodedPartitionValue"
+ } else if (slashSeparateDates) {
+ slashSeparateDateValue(encodedPartitionValue)
+ } else {
+ encodedPartitionValue
+ }
}.mkString("/")
}
+ /**
+ * Turns a `yyyy-MM-dd` formatted partition value into the `yyyy/MM/dd`
directory structure
+ * requested by `hoodie.datasource.write.slash.separated.date.partitioning`,
mirroring the
+ * substitution the write path performs in
`KeyGenUtils#getRecordPartitionPath`.
+ *
+ * A value with a leading dash is returned as-is: substituting would make
the partition path start
+ * with "/", and an absolute relative-partition-path is resolved
inconsistently -- the writer and
+ * the file-system view disagree on where such a partition lives. Such a
value is not a date to
+ * begin with, so nothing is lost by not slashing it.
+ */
+ private def slashSeparateDateValue(partitionValue: String): String = {
+ if (partitionValue.startsWith("-")) partitionValue else
partitionValue.replace('-', '/')
+ }
Review Comment:
🤖 The write path (`KeyGenUtils#getRecordPartitionPath` and
`PartitionPathFormatterBase#combine`) replaces dashes unconditionally, so a
single-field value like `-01-05` would be written to `.../01/05` (leading
slash). Here you deliberately return leading-dash values as-is, so for that
input the DDL would name a different directory than the writer and reintroduce
the silent no-op this PR fixes. I get the reasoning (a leading-slash relative
path is itself broken), but since the stated goal is to name the exact
directory the writer created, could you confirm this asymmetry is intentional
and worth the divergence versus just matching the writer verbatim?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/spark/sql/hudi/HoodieSqlCommonUtils.scala:
##########
@@ -406,24 +406,55 @@ object HoodieSqlCommonUtils extends SparkAdapterSupport {
private def makePartitionPath(partitionFields: Seq[String],
normalizedSpecs: Map[String, String],
enableEncodeUrl: Boolean,
- enableHiveStylePartitioning: Boolean): String
= {
+ enableHiveStylePartitioning: Boolean,
+ slashSeparatedDatePartitioning: Boolean):
String = {
+ // NOTE: Slash-separated date partitioning only kicks in for a table
partitioned by a single
+ // (date) column, mirroring the guard in
[[KeyGenUtils#getRecordPartitionPath]] that drives
+ // the write path -- these commands have to name the very same
directory the writer created.
+ // Hive-style partitioning is excluded because the config documents
the two as mutually
+ // exclusive, and the write paths do not agree on what the
combination should produce
+ // (tracked in HUDI issue #19669), so there is no single directory
to name here
+ val slashSeparateDates =
+ slashSeparatedDatePartitioning && !enableHiveStylePartitioning &&
partitionFields.length == 1
partitionFields.map { partitionColumn =>
val encodedPartitionValue = if (enableEncodeUrl) {
PartitionPathEncodeUtils.escapePathName(normalizedSpecs(partitionColumn))
} else {
normalizedSpecs(partitionColumn)
}
- if (enableHiveStylePartitioning)
s"$partitionColumn=$encodedPartitionValue" else encodedPartitionValue
+ if (enableHiveStylePartitioning) {
+ s"$partitionColumn=$encodedPartitionValue"
+ } else if (slashSeparateDates) {
+ slashSeparateDateValue(encodedPartitionValue)
+ } else {
+ encodedPartitionValue
+ }
}.mkString("/")
}
+ /**
+ * Turns a `yyyy-MM-dd` formatted partition value into the `yyyy/MM/dd`
directory structure
+ * requested by `hoodie.datasource.write.slash.separated.date.partitioning`,
mirroring the
+ * substitution the write path performs in
`KeyGenUtils#getRecordPartitionPath`.
+ *
+ * A value with a leading dash is returned as-is: substituting would make
the partition path start
+ * with "/", and an absolute relative-partition-path is resolved
inconsistently -- the writer and
+ * the file-system view disagree on where such a partition lives. Such a
value is not a date to
+ * begin with, so nothing is lost by not slashing it.
+ */
+ private def slashSeparateDateValue(partitionValue: String): String = {
+ if (partitionValue.startsWith("-")) partitionValue else
partitionValue.replace('-', '/')
Review Comment:
🤖 nit: `slashSeparateDateValue` reads ambiguously — it could be a noun ("the
value of a slash-separated date") rather than a transformation. Something like
`toSlashSeparatedDate` or `dashedToSlashedDate` would make the intent clearer
at the call site.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
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]
