yihua commented on code in PR #711:
URL: https://github.com/apache/hudi-rs/pull/711#discussion_r3928751624
##########
crates/datafusion/tests/plan_tests.rs:
##########
@@ -148,26 +148,37 @@ async fn verify_plan(
plan.contains("SortExec: TopK(fetch=10)"),
"Plan should contain TopK sort"
);
+ // The projection, struct field access included, is pushed into the Parquet
+ // source rather than planned as its own `ProjectionExec`, so it is
asserted
+ // wherever it lands. The index in `structField@N` is the field's position
in
+ // the file schema and differs per table, hence the two-part match.
assert!(
- plan.contains(&format!(
- "ProjectionExec: expr=[id@0 as id, name@1 as name, isActive@2 as
isActive, \
- get_field(structField@3, field2) as
{table_name}.structField[field2]]"
- )),
- "Plan should contain expected projection"
+ plan.contains("get_field(structField@")
Review Comment:
Done. Both anchored: the projection assertion now matches `projection=[id,
name, isActive, get_field(structField@`, so a scan that stopped projecting
fails it, and the filter fragment is checked against a `scan_predicate` helper
that slices out `, predicate=` and cuts before `, pruning_predicate=`. Applied
to the v8 assertion too.
##########
crates/datafusion/tests/plan_tests.rs:
##########
@@ -148,26 +148,37 @@ async fn verify_plan(
plan.contains("SortExec: TopK(fetch=10)"),
"Plan should contain TopK sort"
);
+ // The projection, struct field access included, is pushed into the Parquet
+ // source rather than planned as its own `ProjectionExec`, so it is
asserted
+ // wherever it lands. The index in `structField@N` is the field's position
in
+ // the file schema and differs per table, hence the two-part match.
assert!(
- plan.contains(&format!(
- "ProjectionExec: expr=[id@0 as id, name@1 as name, isActive@2 as
isActive, \
- get_field(structField@3, field2) as
{table_name}.structField[field2]]"
- )),
- "Plan should contain expected projection"
+ plan.contains("get_field(structField@")
+ && plan.contains(&format!(", field2) as
{table_name}.structField[field2]")),
+ "Plan should project the struct field"
);
- // With pushdown_filters enabled, simple predicates (id % 2 = 0, name !=
Alice)
- // are pushed into the Parquet source. Only non-pushable predicates like
- // struct field access remain in FilterExec.
+ // Simple predicates (id % 2 = 0, name != Alice) and the struct field
access
+ // alike are pushed into the Parquet source.
assert!(
- plan.contains("get_field(structField@3, field2) > 30"),
- "Plan should contain struct field filter (either in FilterExec or
DataSourceExec)"
+ plan.contains(", field2) > 30"),
+ "Plan should contain struct field filter"
);
assert!(
-
plan.contains(&format!("input_partitions={planned_input_partitioned}")),
- "Plan should contain expected
input_partitions={planned_input_partitioned}"
+ plan.contains(&format!(
+ "file_groups={{{planned_input_partitioned} {}:",
+ file_group_noun(*planned_input_partitioned)
+ )),
+ "Plan should scan {planned_input_partitioned} file group(s)"
);
}
+/// How the plan spells the file-group count: `hoodie.read.input.partitions`
+/// decides how many groups the scan is split into, and DataFusion renders one
+/// group in the singular.
+fn file_group_noun(count: i32) -> &'static str {
Review Comment:
Done, helper dropped. `file_groups={N group` covers both renderings on its
own.
##########
python/src/datafusion_internal.rs:
##########
@@ -40,23 +40,26 @@ fn extract_codec(session: Bound<PyAny>) ->
PyResult<FFI_LogicalExtensionCodec> {
} else {
session
};
- let capsule = capsule_obj.downcast::<PyCapsule>()?;
- if let Some(name) = capsule.name()? {
- let name = name
- .to_str()
- .map_err(|e| PyValueError::new_err(format!("{e}")))?;
- if name != "datafusion_logical_extension_codec" {
- return Err(PyValueError::new_err(format!(
- "Expected PyCapsule name 'datafusion_logical_extension_codec',
got '{name}'"
- )));
- }
- }
- let codec = unsafe { capsule.reference::<FFI_LogicalExtensionCodec>() };
+ let capsule = capsule_obj.cast::<PyCapsule>()?;
+ // `pointer_checked` is the name check and the pointer read in one step: it
+ // refuses any capsule not carrying exactly this name, which is what makes
+ // the cast below sound. An unnamed capsule no longer passes, where the
+ // previous name-then-read pair let one through.
+ let codec = capsule
+ .pointer_checked(Some(c"datafusion_logical_extension_codec"))
+ .map_err(|e| {
+ PyValueError::new_err(format!(
+ "Expected a PyCapsule named
'datafusion_logical_extension_codec': {e}"
+ ))
+ })?;
+ // SAFETY: DataFusion puts that name only on a capsule holding an
Review Comment:
Good catch, the comment overstated it. Softened rather than adding a check:
`version` sits near the end of `FFI_LogicalExtensionCodec`, so reading it
already assumes the layout it would be verifying, and datafusion-ffi itself
only calls it in its own module-loading tests. The comment now says the name
settles what the capsule holds but not which `datafusion-ffi` built it, and
names the pinned `datafusion` extra as what keeps them in step.
--
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]
