weiqingy commented on code in PR #2424:
URL: https://github.com/apache/auron/pull/2424#discussion_r3660681091
##########
native-engine/datafusion-ext-plans/src/common/row_null_checker.rs:
##########
@@ -51,18 +60,33 @@ impl RowNullChecker {
/// * `NullBuffer` - Buffer indicating which rows contain null values
/// - `false` bits indicate rows that contain at least one null value
/// - `true` bits indicate rows where all fields are non-null
Review Comment:
nit: `row_converter` isn't in the `# Parameters` list, and it carries a
precondition that only shows up at runtime: it has to be the converter that
produced `rows`, or `convert_rows` trips `assert!(Arc::ptr_eq(...), "rows were
not produced by this RowConverter")` (`arrow-row/src/lib.rs:701-704`). All
three producers get it right today, but a fourth would find out the hard way.
Worth spelling out in the doc?
Same block, also a nit: `true` bits indicate rows where all fields are
non-null reads a little ambiguously against the semantics in the issue, since a
non-null struct with nested nulls stays valid. Maybe "top-level key fields"?
--
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]
