hudi-agent commented on code in PR #19028:
URL: https://github.com/apache/hudi/pull/19028#discussion_r3425801025
##########
hudi-common/src/main/java/org/apache/hudi/common/config/HoodieCommonConfig.java:
##########
@@ -83,6 +83,15 @@ public class HoodieCommonConfig extends HoodieConfig {
+ " operation will fail schema compatibility check. Set this option
to true will make the missing "
+ " column be filled with null values to successfully complete the
write operation.");
+ public static final ConfigProperty<Boolean>
ALLOW_TIMESTAMP_PRECISION_EVOLUTION = ConfigProperty
+ .key("hoodie.write.schema.allow.timestamp.precision.evolution")
+ .defaultValue(false)
+ .markAdvanced()
+ .sinceVersion("1.3.0")
+ .withDocumentation("Controls whether schema evolution may change a
column between timestamp-millis and "
Review Comment:
🤖 Should the docs make it explicit that this only relabels the logical type
— it does not rewrite stored values? Since old base files keep their original
encoding, enabling the gate when actual values don't match the new precision
can lead to misinterpretation on read. A one-liner like "Stored values are not
rewritten; only enable when the actual values already match the target
precision" would steer users away from misuse.
<sub><i>- AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-common/src/main/java/org/apache/hudi/internal/schema/utils/SchemaChangeUtils.java:
##########
@@ -52,29 +52,39 @@ public class SchemaChangeUtils {
* @param dst new column type.
* @return whether to allow the column type to be updated.
*/
- public static boolean isTypeUpdateAllow(Type src, Type dst) {
+ /**
Review Comment:
🤖 The PR description notes "Pre-existing overloads kept as delegates," which
is followed for `reconcileSchema` and `ColumnUpdateChange.get`, but here the
2-arg `isTypeUpdateAllow(Type, Type)` is replaced rather than kept as a
delegate. Could you preserve the original 2-arg signature as a thin delegate
(`return isTypeUpdateAllow(src, dst, false);`)? This class is a public utility
in `internal.schema.utils` and could plausibly be called by downstream tooling.
<sub><i>- AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/HoodieSchemaUtils.scala:
##########
@@ -205,7 +209,10 @@ object HoodieSchemaUtils {
// Apply schema evolution, by auto-merging write schema and read schema
val setNullForMissingColumns =
opts.getOrElse(HoodieCommonConfig.SET_NULL_FOR_MISSING_COLUMNS.key(),
HoodieCommonConfig.SET_NULL_FOR_MISSING_COLUMNS.defaultValue()).toBoolean
- val mergedInternalSchema =
AvroSchemaEvolutionUtils.reconcileSchema(canonicalizedSourceSchema.toAvroSchema(),
internalSchema, setNullForMissingColumns)
+ val allowTimestampPrecisionEvolution =
opts.getOrElse(HoodieCommonConfig.ALLOW_TIMESTAMP_PRECISION_EVOLUTION.key,
Review Comment:
🤖 nit: `allowTimestampPrecisionEvolution` is already declared from `opts` at
the outer scope a few dozen lines above — could you just reference that val
here instead of re-reading the same key with the same default?
<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]
