linliu-code commented on code in PR #19709:
URL: https://github.com/apache/hudi/pull/19709#discussion_r3846599532


##########
hudi-spark-datasource/hudi-spark-common/src/test/scala/org/apache/spark/sql/hudi/command/TestSqlKeyGenerator.scala:
##########
@@ -0,0 +1,213 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.hudi.command
+
+import org.apache.hudi.common.config.TimestampKeyGeneratorConfig
+import org.apache.hudi.common.config.TypedProperties
+import org.apache.hudi.common.util.PartitionPathEncodeUtils
+import org.apache.hudi.exception.HoodieKeyException
+import org.apache.hudi.keygen.{SimpleKeyGenerator, TimestampBasedKeyGenerator}
+import org.apache.hudi.keygen.constant.KeyGeneratorOptions
+
+import org.apache.avro.Schema
+import org.apache.avro.generic.GenericData
+import org.joda.time.DateTimeZone
+import org.junit.jupiter.api.Assertions.{assertEquals, assertThrows}
+import org.junit.jupiter.api.Test
+
+import scala.collection.JavaConverters._
+
+/**
+ * Tests that [[SqlKeyGenerator]] resolves a partition path without also 
requiring the record key.
+ *
+ * MOR partial updates materialise the merged record against 
`WRITE_PARTIAL_UPDATE_SCHEMA`, which
+ * carries only the fields named in `UPDATE SET`. 
`HoodieIndexUtils#inferPartitionPath` then asks
+ * the key generator for that record's partition path, so a record key absent 
from the assignments
+ * is legitimately unset at that point and must not fail partition resolution.
+ */
+class TestSqlKeyGenerator {
+
+  private val schema = new Schema.Parser().parse(
+    """
+       |{
+       |  "type": "record",
+       |  "name": "test_record",
+       |  "fields": [
+       |    {"name": "id", "type": ["null", "long"], "default": null},
+       |    {"name": "amount", "type": ["null", "double"], "default": null},
+       |    {"name": "dt", "type": ["null", "string"], "default": null}
+       |  ]
+       |}
+     """.stripMargin)
+
+  /** The same record shape with only the named fields, as a partial update 
produces. */
+  private def projected(fieldNames: String*): Schema = {
+    val fields = schema.getFields.asScala
+      .filter(f => fieldNames.contains(f.name))
+      .map(f => new Schema.Field(f.name, f.schema, null, f.defaultVal))
+    Schema.createRecord("test_record", null, null, false, fields.asJava)
+  }
+
+  private def keyGenerator(partitionSchema: Option[String] = None,

Review Comment:
   Done, and thank you, this one invalidated most of the round. Took the 
suggestion: the default is now `Some("dt string")`, with one explicit `None` 
case kept for the non-partitioned table.
   
   I verified the claim rather than taking it: 
`ProvidesHoodieConfig.scala:74,101,274,425` all set 
`SqlKeyGenerator.PARTITION_SCHEMA` as unconditional map entries, so `None` 
modelled a non-partitioned table and nothing else. The consequence was as you 
said, seven of the eight cases returned at line 200 before the guard, including 
the two named after default-partition behaviour.
   
   The `None` case is now paired against the timestamp case on the *same* 
micros value, so it asserts the conversion is skipped (`1786406400000000` comes 
back verbatim) rather than merely being absent.



##########
hudi-spark-datasource/hudi-spark-common/src/test/scala/org/apache/spark/sql/hudi/command/TestSqlKeyGenerator.scala:
##########
@@ -0,0 +1,213 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.hudi.command
+
+import org.apache.hudi.common.config.TimestampKeyGeneratorConfig
+import org.apache.hudi.common.config.TypedProperties
+import org.apache.hudi.common.util.PartitionPathEncodeUtils
+import org.apache.hudi.exception.HoodieKeyException
+import org.apache.hudi.keygen.{SimpleKeyGenerator, TimestampBasedKeyGenerator}
+import org.apache.hudi.keygen.constant.KeyGeneratorOptions
+
+import org.apache.avro.Schema
+import org.apache.avro.generic.GenericData
+import org.joda.time.DateTimeZone
+import org.junit.jupiter.api.Assertions.{assertEquals, assertThrows}
+import org.junit.jupiter.api.Test
+
+import scala.collection.JavaConverters._
+
+/**
+ * Tests that [[SqlKeyGenerator]] resolves a partition path without also 
requiring the record key.
+ *
+ * MOR partial updates materialise the merged record against 
`WRITE_PARTIAL_UPDATE_SCHEMA`, which
+ * carries only the fields named in `UPDATE SET`. 
`HoodieIndexUtils#inferPartitionPath` then asks
+ * the key generator for that record's partition path, so a record key absent 
from the assignments
+ * is legitimately unset at that point and must not fail partition resolution.
+ */
+class TestSqlKeyGenerator {
+
+  private val schema = new Schema.Parser().parse(
+    """
+       |{
+       |  "type": "record",
+       |  "name": "test_record",
+       |  "fields": [
+       |    {"name": "id", "type": ["null", "long"], "default": null},
+       |    {"name": "amount", "type": ["null", "double"], "default": null},
+       |    {"name": "dt", "type": ["null", "string"], "default": null}
+       |  ]
+       |}
+     """.stripMargin)
+
+  /** The same record shape with only the named fields, as a partial update 
produces. */
+  private def projected(fieldNames: String*): Schema = {
+    val fields = schema.getFields.asScala
+      .filter(f => fieldNames.contains(f.name))
+      .map(f => new Schema.Field(f.name, f.schema, null, f.defaultVal))
+    Schema.createRecord("test_record", null, null, false, fields.asJava)
+  }
+
+  private def keyGenerator(partitionSchema: Option[String] = None,
+                           withRecordKey: Boolean = true): SqlKeyGenerator = {
+    val props = new TypedProperties()
+    props.put(SqlKeyGenerator.ORIGINAL_KEYGEN_CLASS_NAME, 
classOf[SimpleKeyGenerator].getName)
+    if (withRecordKey) {
+      props.put(KeyGeneratorOptions.RECORDKEY_FIELD_NAME.key, "id")
+    }
+    props.put(KeyGeneratorOptions.PARTITIONPATH_FIELD_NAME.key, "dt")
+    // Spark SQL always sets this (see MergeIntoHoodieTableCommand), and it is 
what drives
+    // convertPartitionPathToSqlType, so cover it rather than leaving it None.
+    partitionSchema.foreach(ps => props.put(SqlKeyGenerator.PARTITION_SCHEMA, 
ps))
+    new SqlKeyGenerator(props)
+  }
+
+  /** Delegates to a TimestampBasedKeyGenerator rather than a 
SimpleKeyGenerator. */
+  private def timestampKeyGenerator(partitionSchema: Option[String] = None): 
SqlKeyGenerator = {
+    val props = new TypedProperties()
+    props.put(SqlKeyGenerator.ORIGINAL_KEYGEN_CLASS_NAME, 
classOf[TimestampBasedKeyGenerator].getName)
+    props.put(KeyGeneratorOptions.RECORDKEY_FIELD_NAME.key, "id")
+    props.put(KeyGeneratorOptions.PARTITIONPATH_FIELD_NAME.key, "dt")
+    props.put(TimestampKeyGeneratorConfig.TIMESTAMP_TYPE_FIELD.key, 
"DATE_STRING")
+    props.put(TimestampKeyGeneratorConfig.TIMESTAMP_INPUT_DATE_FORMAT.key, 
"yyyy-MM-dd")
+    props.put(TimestampKeyGeneratorConfig.TIMESTAMP_OUTPUT_DATE_FORMAT.key, 
"yyyy-MM-dd")
+    partitionSchema.foreach(ps => props.put(SqlKeyGenerator.PARTITION_SCHEMA, 
ps))
+    new SqlKeyGenerator(props)
+  }
+
+  /** The partition column is populated; only the record key is missing, as 
under a partial update. */
+  private def recordWithoutRecordKey: GenericData.Record = {
+    val record = new GenericData.Record(schema)
+    record.put("amount", 15.0d)
+    record.put("dt", "2026-08-11")
+    record
+  }
+
+  @Test
+  def testGetPartitionPathDoesNotRequireRecordKey(): Unit = {
+    // Before the fix this threw HoodieKeyException, because getPartitionPath 
delegated to
+    // BaseKeyGenerator#getKey, which builds the whole HoodieKey and so 
validates the record key.
+    assertEquals("2026-08-11", 
keyGenerator().getPartitionPath(recordWithoutRecordKey))
+  }
+
+  @Test
+  def testGetRecordKeyStillRejectsAMissingRecordKey(): Unit = {
+    // Scope guard: the fix must not weaken record-key validation, only stop 
getPartitionPath from
+    // triggering it. A record key that is genuinely required and absent is 
still an error.
+    assertThrows(classOf[HoodieKeyException], () => 
keyGenerator().getRecordKey(recordWithoutRecordKey))
+  }
+
+  @Test
+  def testGetPartitionPathAndRecordKeyOnACompleteRecord(): Unit = {
+    val record = recordWithoutRecordKey
+    record.put("id", 1L)
+    assertEquals("2026-08-11", keyGenerator().getPartitionPath(record))
+    assertEquals("1", keyGenerator().getRecordKey(record))
+  }
+
+  /**
+   * Drives convertPartitionPathToSqlType's TimestampType arm, which is the 
only arm that does any
+   * work: a `dt string` partition schema takes the identity case, so it pins 
nothing. The value is
+   * microseconds because that is what the GenericRecord path assumes when
+   * hoodie.datasource.write.keygenerator.consistent.logical.timestamp.enabled 
is off. Timezone is
+   * pinned because the output is formatted in the default zone.
+   */
+  @Test
+  def testGetPartitionPathConvertsATimestampPartitionValue(): Unit = {
+    val previousZone = DateTimeZone.getDefault
+    DateTimeZone.setDefault(DateTimeZone.UTC)
+    try {
+      val record = new GenericData.Record(schema)
+      record.put("id", 1L)
+      // 2026-08-11 00:00:00 UTC expressed in microseconds.
+      record.put("dt", String.valueOf(1786406400000000L))
+      assertEquals("2026-08-11 00%3A00%3A00", keyGenerator(Some("dt 
timestamp")).getPartitionPath(record))
+    } finally {
+      DateTimeZone.setDefault(previousZone)
+    }
+  }
+
+  /**
+   * A TimestampBasedKeyGenerator delegate does NOT substitute 
HUDI_DEFAULT_PARTITION_PATH for an
+   * absent partition field: it formats the epoch instead, so the HUDI-8315 
guard in
+   * convertPartitionPathToSqlType never fires for it. Pinned because this 
change makes the case
+   * reachable, where previously the record-key exception pre-empted it.
+   */
+  @Test
+  def testTimestampDelegateResolvesAnAbsentPartitionFieldToTheEpoch(): Unit = {
+    val previousZone = DateTimeZone.getDefault
+    DateTimeZone.setDefault(DateTimeZone.UTC)
+    try {
+      val record = new GenericData.Record(projected("id", "amount"))
+      record.put("id", 1L)
+      record.put("amount", 15.0d)
+      assertEquals("1970-01-01", 
timestampKeyGenerator().getPartitionPath(record))

Review Comment:
   Done. All three spellings are now pinned as separate cases, since as you say 
the delegate diverges under each: `None` and `dt string` both resolve to 
`1970-01-01`, and `dt timestamp` throws.
   
   On the `dt timestamp` case I am pinning `NumberFormatException` as the 
behaviour that exists today, not as the behaviour that should exist. A bare 
`NumberFormatException` out of `_partitionValue.toLong` is a poor diagnostic, 
and if you would rather that be wrapped, I am happy to do it here or file it; 
the test would then change with it.



##########
hudi-spark-datasource/hudi-spark-common/src/test/scala/org/apache/spark/sql/hudi/command/TestSqlKeyGenerator.scala:
##########
@@ -0,0 +1,213 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.hudi.command
+
+import org.apache.hudi.common.config.TimestampKeyGeneratorConfig
+import org.apache.hudi.common.config.TypedProperties
+import org.apache.hudi.common.util.PartitionPathEncodeUtils
+import org.apache.hudi.exception.HoodieKeyException
+import org.apache.hudi.keygen.{SimpleKeyGenerator, TimestampBasedKeyGenerator}
+import org.apache.hudi.keygen.constant.KeyGeneratorOptions
+
+import org.apache.avro.Schema
+import org.apache.avro.generic.GenericData
+import org.joda.time.DateTimeZone
+import org.junit.jupiter.api.Assertions.{assertEquals, assertThrows}
+import org.junit.jupiter.api.Test
+
+import scala.collection.JavaConverters._
+
+/**
+ * Tests that [[SqlKeyGenerator]] resolves a partition path without also 
requiring the record key.
+ *
+ * MOR partial updates materialise the merged record against 
`WRITE_PARTIAL_UPDATE_SCHEMA`, which
+ * carries only the fields named in `UPDATE SET`. 
`HoodieIndexUtils#inferPartitionPath` then asks
+ * the key generator for that record's partition path, so a record key absent 
from the assignments
+ * is legitimately unset at that point and must not fail partition resolution.
+ */
+class TestSqlKeyGenerator {
+
+  private val schema = new Schema.Parser().parse(
+    """
+       |{
+       |  "type": "record",
+       |  "name": "test_record",
+       |  "fields": [
+       |    {"name": "id", "type": ["null", "long"], "default": null},
+       |    {"name": "amount", "type": ["null", "double"], "default": null},
+       |    {"name": "dt", "type": ["null", "string"], "default": null}
+       |  ]
+       |}
+     """.stripMargin)
+
+  /** The same record shape with only the named fields, as a partial update 
produces. */
+  private def projected(fieldNames: String*): Schema = {
+    val fields = schema.getFields.asScala
+      .filter(f => fieldNames.contains(f.name))
+      .map(f => new Schema.Field(f.name, f.schema, null, f.defaultVal))
+    Schema.createRecord("test_record", null, null, false, fields.asJava)
+  }
+
+  private def keyGenerator(partitionSchema: Option[String] = None,
+                           withRecordKey: Boolean = true): SqlKeyGenerator = {
+    val props = new TypedProperties()
+    props.put(SqlKeyGenerator.ORIGINAL_KEYGEN_CLASS_NAME, 
classOf[SimpleKeyGenerator].getName)
+    if (withRecordKey) {
+      props.put(KeyGeneratorOptions.RECORDKEY_FIELD_NAME.key, "id")
+    }
+    props.put(KeyGeneratorOptions.PARTITIONPATH_FIELD_NAME.key, "dt")
+    // Spark SQL always sets this (see MergeIntoHoodieTableCommand), and it is 
what drives
+    // convertPartitionPathToSqlType, so cover it rather than leaving it None.
+    partitionSchema.foreach(ps => props.put(SqlKeyGenerator.PARTITION_SCHEMA, 
ps))
+    new SqlKeyGenerator(props)
+  }
+
+  /** Delegates to a TimestampBasedKeyGenerator rather than a 
SimpleKeyGenerator. */
+  private def timestampKeyGenerator(partitionSchema: Option[String] = None): 
SqlKeyGenerator = {
+    val props = new TypedProperties()
+    props.put(SqlKeyGenerator.ORIGINAL_KEYGEN_CLASS_NAME, 
classOf[TimestampBasedKeyGenerator].getName)
+    props.put(KeyGeneratorOptions.RECORDKEY_FIELD_NAME.key, "id")
+    props.put(KeyGeneratorOptions.PARTITIONPATH_FIELD_NAME.key, "dt")
+    props.put(TimestampKeyGeneratorConfig.TIMESTAMP_TYPE_FIELD.key, 
"DATE_STRING")
+    props.put(TimestampKeyGeneratorConfig.TIMESTAMP_INPUT_DATE_FORMAT.key, 
"yyyy-MM-dd")
+    props.put(TimestampKeyGeneratorConfig.TIMESTAMP_OUTPUT_DATE_FORMAT.key, 
"yyyy-MM-dd")
+    partitionSchema.foreach(ps => props.put(SqlKeyGenerator.PARTITION_SCHEMA, 
ps))
+    new SqlKeyGenerator(props)
+  }
+
+  /** The partition column is populated; only the record key is missing, as 
under a partial update. */
+  private def recordWithoutRecordKey: GenericData.Record = {
+    val record = new GenericData.Record(schema)
+    record.put("amount", 15.0d)
+    record.put("dt", "2026-08-11")
+    record
+  }
+
+  @Test
+  def testGetPartitionPathDoesNotRequireRecordKey(): Unit = {
+    // Before the fix this threw HoodieKeyException, because getPartitionPath 
delegated to
+    // BaseKeyGenerator#getKey, which builds the whole HoodieKey and so 
validates the record key.
+    assertEquals("2026-08-11", 
keyGenerator().getPartitionPath(recordWithoutRecordKey))
+  }
+
+  @Test
+  def testGetRecordKeyStillRejectsAMissingRecordKey(): Unit = {
+    // Scope guard: the fix must not weaken record-key validation, only stop 
getPartitionPath from
+    // triggering it. A record key that is genuinely required and absent is 
still an error.
+    assertThrows(classOf[HoodieKeyException], () => 
keyGenerator().getRecordKey(recordWithoutRecordKey))
+  }
+
+  @Test
+  def testGetPartitionPathAndRecordKeyOnACompleteRecord(): Unit = {
+    val record = recordWithoutRecordKey
+    record.put("id", 1L)
+    assertEquals("2026-08-11", keyGenerator().getPartitionPath(record))
+    assertEquals("1", keyGenerator().getRecordKey(record))
+  }
+
+  /**
+   * Drives convertPartitionPathToSqlType's TimestampType arm, which is the 
only arm that does any
+   * work: a `dt string` partition schema takes the identity case, so it pins 
nothing. The value is
+   * microseconds because that is what the GenericRecord path assumes when
+   * hoodie.datasource.write.keygenerator.consistent.logical.timestamp.enabled 
is off. Timezone is
+   * pinned because the output is formatted in the default zone.
+   */
+  @Test
+  def testGetPartitionPathConvertsATimestampPartitionValue(): Unit = {
+    val previousZone = DateTimeZone.getDefault
+    DateTimeZone.setDefault(DateTimeZone.UTC)
+    try {
+      val record = new GenericData.Record(schema)
+      record.put("id", 1L)
+      // 2026-08-11 00:00:00 UTC expressed in microseconds.
+      record.put("dt", String.valueOf(1786406400000000L))
+      assertEquals("2026-08-11 00%3A00%3A00", keyGenerator(Some("dt 
timestamp")).getPartitionPath(record))
+    } finally {
+      DateTimeZone.setDefault(previousZone)
+    }
+  }
+
+  /**
+   * A TimestampBasedKeyGenerator delegate does NOT substitute 
HUDI_DEFAULT_PARTITION_PATH for an
+   * absent partition field: it formats the epoch instead, so the HUDI-8315 
guard in
+   * convertPartitionPathToSqlType never fires for it. Pinned because this 
change makes the case
+   * reachable, where previously the record-key exception pre-empted it.
+   */
+  @Test
+  def testTimestampDelegateResolvesAnAbsentPartitionFieldToTheEpoch(): Unit = {
+    val previousZone = DateTimeZone.getDefault
+    DateTimeZone.setDefault(DateTimeZone.UTC)
+    try {
+      val record = new GenericData.Record(projected("id", "amount"))
+      record.put("id", 1L)
+      record.put("amount", 15.0d)
+      assertEquals("1970-01-01", 
timestampKeyGenerator().getPartitionPath(record))
+    } finally {
+      DateTimeZone.setDefault(previousZone)
+    }
+  }
+
+  /**
+   * Without a record-key config KeyGenUtils#isAutoGeneratedRecordKeysEnabled 
is true, so the delegate
+   * is wrapped in an AutoRecordGenWrapperKeyGenerator. That wrapper is a 
BaseKeyGenerator, so it now
+   * takes the direct arm and getPartitionPath no longer consumes a generated 
sequence id as a side
+   * effect of building a HoodieKey. Uniqueness does not depend on the stride, 
but the change is
+   * pinned here rather than left as an unexercised side effect.
+   */
+  @Test
+  def testAutoRecordKeyDelegateStillResolvesThePartitionPath(): Unit = {
+    val record = new GenericData.Record(schema)
+    record.put("amount", 15.0d)
+    record.put("dt", "2026-08-11")
+    val keyGen = keyGenerator(withRecordKey = false)
+    assertEquals("2026-08-11", keyGen.getPartitionPath(record))

Review Comment:
   Done, and you were right about the reason it was red. On base the failure 
was `IllegalArgumentException: Property _hoodie.record.key.gen.partition.id not 
found`, i.e. my fixture, not the behaviour the scaladoc claimed to pin. 
Confirmed production always sets both: `HoodieCreateRecordUtils.scala:130-131` 
and `181-182`.
   
   `keyGenerator()` now sets `RECORD_KEY_GEN_INSTANT_TIME_CONFIG` and 
`RECORD_KEY_GEN_PARTITION_ID_CONFIG`, and the case asserts the sequence id 
after two partition lookups. Your predicted values reproduce exactly: `100_1_0` 
here, and on the parent commit `expected: <100_1_0> but was: <100_1_2>`.
   
   One deviation from `TestCreateKeyGeneratorByTypeWithFactory`: the partition 
id is set as the string `"1"` rather than the int. The int literal compiles on 
this base but not on the 1.x line, where scalac rejects the implicit conversion 
to `Object`; both `TypedProperties` variants render the value through 
`String.valueOf` on read, so it is equivalent at runtime.



##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/spark/sql/hudi/command/SqlKeyGenerator.scala:
##########
@@ -99,7 +99,15 @@ class SqlKeyGenerator(props: TypedProperties) extends 
BuiltinKeyGenerator(props)
 
   override def getPartitionPath(record: GenericRecord): String = {

Review Comment:
   Added. Both Avro arms of `MergeIntoKeyGenerator` now check 
`record.getSchema.getFields.size` against the ordinal before reading it, 
falling through to the existing fallback. New `TestMergeIntoKeyGenerator` 
covers both short-record arms, and on the parent commit they fail exactly as 
you predicted: `ArrayIndexOutOfBoundsException: Index 3 out of bounds for 
length 2` from `MergeIntoKeyGenerator.scala:69` for the partition path, and 
`expected HoodieKeyException but was ArrayIndexOutOfBoundsException` for the 
record key.
   
   Two things to push back on if you disagree:
   
   1. It is a bounds check, not a meta-field *name* check. A name check at the 
ordinal would be strictly more correct, but it would additionally change 
behaviour for a long record whose schema is not meta-prefixed, which is beyond 
the reported defect. Said so in the scaladoc. Say the word and I will make it a 
name check instead.
   2. I added two meta-prefixed regression cases alongside. Without them the 
guard could disable meta-field resolution outright and both short-record cases 
would still pass, which is the failure mode you caught me in elsewhere in this 
review.



##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/spark/sql/hudi/command/SqlKeyGenerator.scala:
##########
@@ -99,7 +99,15 @@ class SqlKeyGenerator(props: TypedProperties) extends 
BuiltinKeyGenerator(props)
 
   override def getPartitionPath(record: GenericRecord): String = {
     val partitionPath = originalKeyGen.map {

Review Comment:
   Dropped, `getRecordKey(GenericRecord)` is back to the `getKey` form. Your 
reasoning is the deciding one: it is a second behaviour change unrelated to 
what #19708 reports, and with this PR already gated on #19713 for merge order 
there is no reason to widen it.
   
   Carrying it and the case that discriminates it (your other comment: 
`TimestampBasedKeyGenerator` with `dt = "not-a-date"`, base throws, head 
returns `"1"`) into the follow-up together, so the change and its test land as 
one reviewable unit.



##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/spark/sql/hudi/command/SqlKeyGenerator.scala:
##########
@@ -77,7 +77,13 @@ class SqlKeyGenerator(props: TypedProperties) extends 
BuiltinKeyGenerator(props)
 
   override def getRecordKey(record: GenericRecord): String =
     originalKeyGen.map {
-      _.getKey(record).getRecordKey
+      // Mirror of getPartitionPath below: resolve the record key alone where 
the generator exposes
+      // it, so neither accessor pays for the other half. Going through getKey 
would also compute the
+      // partition path and discard it, and would let a partition-side failure 
surface from a
+      // record-key lookup: TimestampBasedAvroKeyGenerator raises 
HoodieKeyGeneratorException
+      // independently of the key.
+      case baseKeyGen: BaseKeyGenerator => baseKeyGen.getRecordKey(record)

Review Comment:
   Deferred with the arm itself. Since `getRecordKey` is reverted to the 
`getKey` form, there is no new behaviour on that path in this PR, so the case 
belongs with the follow-up rather than here, and I noted your `dt = 
"not-a-date"` repro on that thread so it is not lost.
   
   You are right that the existing scope-guard test does not discriminate the 
arm. It was never meant to, it guards against the fix weakening record-key 
validation, and it passes on both sides by design. I should have labelled it 
that way instead of leaving it to look like coverage of the change.



-- 
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]

Reply via email to