yihua commented on code in PR #724:
URL: https://github.com/apache/hudi-rs/pull/724#discussion_r4419908263
##########
crates/core/src/config/table.rs:
##########
@@ -261,10 +261,15 @@ impl ConfigParser for HudiTableConfig {
// A table written without partitioning records the key with an
// empty value rather than omitting it; splitting that yields one
// field named "", which no schema contains.
+ //
+ // `CustomKeyGenerator` annotates each field with its key type
+ // (`region:SIMPLE,ts_str:TIMESTAMP`); the field name is what
+ // precedes the annotation.
Self::PartitionFields => get_result.map(|v| {
HudiConfigValue::List(
v.split(',')
.map(str::trim)
+ .map(|field|
field.split(':').next().unwrap_or_default().trim())
Review Comment:
non-blocking: This stripping, the two custom-keygen fixtures and the
`custom_keygen_partition_paths` tests are really the first piece of #549, and
the description still says CustomKeyGenerator is out of scope here. Would you
consider lifting them into a small follow-up PR that opens #549, so this one
stays the #798 fix and drops back under the 1000-line guideline?
##########
crates/core/src/config/util.rs:
##########
@@ -77,12 +77,96 @@ pub fn parse_data_for_options(data: &Bytes, split_chars:
&str) -> Result<HashMap
.trim()
.to_owned();
let value = parts.next().unwrap_or("").trim().to_owned();
- options.insert(key, value);
+ options.insert(
+ unescape_java_properties(&key),
Review Comment:
Java's `DFSPropertiesConfiguration` splits `hudi-defaults.conf` on the
delimiter without Properties unescaping, so applying it here changes any
backslash-carrying value in the global config relative to Hudi. Would it be
simpler to unescape only in `imbue_table_properties`, since `hoodie.properties`
is the file written by `Properties.store`?
##########
crates/core/src/keygen/timestamp_based.rs:
##########
@@ -597,18 +576,207 @@ mod tests {
TimestampBasedKeyGenerator::java_to_chrono_format("yyyy-MM-dd'T'HH:mm:ss.SSSZ"),
"%Y-%m-%dT%H:%M:%S.%3f%#z"
);
+ }
- // Hive-style: format segments → semantic names
+ #[test]
+ fn test_scalar_time_unit_parsing_and_conversion() {
Review Comment:
non-blocking: The tests from 1e23b576 (scalar units, timezones, epoch
overflow, defensive guards, `is_table_partitioned`, `should_include` fail-open)
cover paths this PR doesn't change. Could they go in a separate `test(core):`
PR? They'd merge on their own, and it keeps this diff to the fix and the tests
that pin it.
##########
crates/core/src/table/partition.rs:
##########
@@ -401,6 +401,136 @@ mod tests {
assert!(!pruner.should_include("date=2023-02-01/category=B/count=10"));
}
+ #[test]
+ fn test_hoodie_partition_path_whole_path_pruning() {
+ // A timestamp key generator rewrites a source-column predicate into a
predicate on
+ // the opaque whole-path meta field, so the pruner holds a
single-field schema and
+ // must compare the path as one string rather than split it into
segments.
+ let configs = HudiConfigs::new([
+ ("hoodie.table.partition.fields", "ts_str"),
+ (
+ "hoodie.table.keygenerator.class",
+ "org.apache.hudi.keygen.TimestampBasedKeyGenerator",
+ ),
+ ("hoodie.keygen.timebased.timestamp.type", "DATE_STRING"),
+ (
+ "hoodie.keygen.timebased.input.dateformat",
+ "yyyy-MM-dd'T'HH:mm:ss.SSSZ",
+ ),
+ ("hoodie.keygen.timebased.output.dateformat", "yyyy/MM/dd/HH"),
+ ("hoodie.datasource.write.hive_style_partitioning", "true"),
+ ("hoodie.datasource.write.partitionpath.urlencode", "false"),
+ ]);
+ let partition_schema = Schema::new(vec![Field::new(
+ MetaField::PartitionPath.as_ref(),
+ DataType::Utf8,
+ false,
+ )]);
+ let filter = Filter::try_from(("ts_str", "=",
"2023-04-01T12:01:00.123Z")).unwrap();
+
+ let pruner = PartitionPruner::new(&[filter], &partition_schema,
&configs).unwrap();
+ assert!(!pruner.is_empty());
+ assert!(pruner.should_include("ts_str=2023/04/01/12"));
+ assert!(!pruner.should_include("ts_str=2023/05/01/08"));
+ }
+
+ #[test]
+ fn test_hoodie_partition_path_whole_path_pruning_url_encoded() {
+ let configs = HudiConfigs::new([
+ ("hoodie.table.partition.fields", "ts_str"),
+ (
+ "hoodie.table.keygenerator.class",
+ "org.apache.hudi.keygen.TimestampBasedKeyGenerator",
+ ),
+ ("hoodie.keygen.timebased.timestamp.type", "DATE_STRING"),
+ (
+ "hoodie.keygen.timebased.input.dateformat",
+ "yyyy-MM-dd'T'HH:mm:ss.SSSZ",
+ ),
+ ("hoodie.keygen.timebased.output.dateformat", "yyyy/MM/dd/HH"),
+ ("hoodie.datasource.write.hive_style_partitioning", "true"),
+ ("hoodie.datasource.write.partitionpath.urlencode", "true"),
+ ]);
+ let partition_schema = Schema::new(vec![Field::new(
+ MetaField::PartitionPath.as_ref(),
+ DataType::Utf8,
+ false,
+ )]);
+ let filter = Filter::try_from(("ts_str", "=",
"2023-04-01T12:01:00.123Z")).unwrap();
+
+ let pruner = PartitionPruner::new(&[filter], &partition_schema,
&configs).unwrap();
+ // The encoded path decodes to `ts_str=2023/04/01/12` before
comparison.
+ assert!(pruner.should_include("ts_str%3D2023%2F04%2F01%2F12"));
Review Comment:
non-blocking: Hudi url-encodes the value before adding the hive prefix
(`escapePathName` then `field + "="`), so the real path here is
`ts_str=2023%2F04%2F01%2F12` rather than `ts_str%3D…`. Worth using that
spelling so the test pins what a writer produces.
--
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]
