ColumnName tracks a path of field names instead of a simple string #445

scovich · 2024-10-29T16:49:30Z

What changes are proposed in this pull request?

Previously, ColumnName tracked a simple string, which could be split at . to obtain a path of field names. But this breaks down if a field name contains any special characters that might interfere with the interpretation of a dot character.

To solve this, update ColumnName to track an actual path of field names instead. Update all call sites as needed to support the new idiom.

This PR also includes code for reliably parsing strings into ColumnName using period as field separator and backticks as delimiters for field names containing special characters, e.g:

assert_eq!(ColumnName::new(["a", "b c", "d"]).to_string(), "a.`b c`.d");

NOTE: This change does not magically make all operations nesting-aware. For example, code that loops over the field names of a StructType will continue to see nested column names as not-matched. Fixing those call sites is left as future work, tho obvious ones are flagged here as TODO.

Resolves #443

This PR affects the following public APIs

The "shape" of ColumnName changes from string-like to slice-of-string-like, and its methods change accordingly. This change is needed because otherwise we cannot reliably handle arbitrary field names.

How was this change tested?

Extensive existing unit tests.

codecov · 2024-10-29T16:53:21Z

Codecov Report

Attention: Patch coverage is 91.64265% with 29 lines in your changes missing coverage. Please review.

Project coverage is 78.41%. Comparing base (0214a96) to head (b9c543e).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
ffi/src/expressions/engine.rs	0.00%	11 Missing ⚠️
kernel/src/expressions/column_names.rs	95.39%	3 Missing and 7 partials ⚠️
kernel/src/engine/parquet_stats_skipping/tests.rs	66.66%	6 Missing ⚠️
ffi/src/expressions/kernel.rs	0.00%	1 Missing ⚠️
kernel/src/engine/parquet_stats_skipping.rs	90.90%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #445      +/-   ##
==========================================
+ Coverage   78.08%   78.41%   +0.33%     
==========================================
  Files          55       55              
  Lines       11605    11806     +201     
  Branches    11605    11806     +201     
==========================================
+ Hits         9062     9258     +196     
+ Misses       2043     2041       -2     
- Partials      500      507       +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

scovich · 2024-10-29T16:53:00Z

ffi/src/expressions/engine.rs

+fn wrap_expression(state: &mut KernelExpressionVisitorState, expr: impl Into<Expression>) -> usize {
+    state.inflight_expressions.insert(expr.into())


This is an opportunistic change that makes the visitor logic simpler
(I noticed it while updating the column name visitor below).

Happy to pull it out as a separate (prefactor) PR if it's unwelcome here, just holler.

I think this is okay here, makes sense for the simplification

scovich · 2024-10-29T16:53:50Z

ffi/src/expressions/kernel.rs

@@ -321,7 +321,13 @@ pub unsafe extern "C" fn visit_expression(
                visit_expression_scalar(visitor, scalar, sibling_list_id)
            }
            Expression::Column(name) => {
-                call!(visitor, visit_column, sibling_list_id, name.as_str().into())
+                // TODO: Support nested columns properly!


Tracked as #423

scovich · 2024-10-29T16:54:03Z

ffi/src/expressions/engine.rs

@@ -149,7 +149,7 @@ fn visit_expression_column_impl(
    name: DeltaResult<String>,
 ) -> DeltaResult<usize> {
    // TODO: FIXME: This is incorrect if any field name in the column path contains a period.


Tracked as #423

scovich · 2024-10-29T16:55:22Z

kernel/src/actions/set_transaction.rs

-            LazyLock::new(|| Some(Arc::new(column_expr!("txn.appId").is_not_null())));
+        static META_PREDICATE: LazyLock<Option<ExpressionRef>> = LazyLock::new(|| {
+            Some(Arc::new(
+                Expr::column([SET_TRANSACTION_NAME, "appId"]).is_not_null(),


This is a bit bulkier than the original code, but it relies less on "magic constant" code.

Do we prefer this new way as somewhat safer? Or prefer the old way as more compact?

(several more below)

I prefer safer myself.

scovich · 2024-10-29T16:57:23Z

kernel/src/engine/parquet_row_group_skipping.rs

-        .filter_map(|(i, f)| requested_columns.take(f.path()).map(|path| (path, i)))
+        .filter_map(|(i, f)| {
+            requested_columns
+                .take(f.path().parts())


Sneaky: HashSet::take accepts Borrow as input, and ColumnName borrows as &[String]. Thus, we can probe the hash set with a parquet schema ColumnPath, and the take returns the corresponding (already-allocated) ColumnName.

kernel/src/expressions/column_names.rs

scovich · 2024-10-29T17:09:40Z

kernel/src/expressions/column_names.rs

    }
 }

 impl Display for ColumnName {
    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
-        (**self).fmt(f)
+        // TODO: Handle non-simple field names better?


Tracked as #444

scovich · 2024-10-29T17:09:56Z

kernel/src/expressions/column_names.rs

+// TODO: Marked as test-only for now, as a demonstration. Should this be Display for ColumnName, so
+// `ColumnName::new(["a", "b.c", "d"])` would print as `"a.[b.c].d"` instead of merely `"a.b.c.d"`?


Tracked as #444

scovich · 2024-10-29T17:10:29Z

kernel/src/expressions/column_names.rs

+/// square brackets may contain arbitrary characters, including periods and spaces. To include a
+/// literal square bracket in a field name, escape it by doubling it, e.g. `"[foo]]bar]"` would
+/// parse as `foo]bar`.
+// TODO: Marked as test-only for now, as a demonstration. Should we officially support it?


Tracked as #444

same comment re [[ as above

kernel/src/expressions/column_names.rs

ffi/src/expressions/kernel.rs

nicklan

flushing a couple comments

nicklan · 2024-10-29T20:01:35Z

kernel/src/actions/set_transaction.rs

-            LazyLock::new(|| Some(Arc::new(column_expr!("txn.appId").is_not_null())));
+        static META_PREDICATE: LazyLock<Option<ExpressionRef>> = LazyLock::new(|| {
+            Some(Arc::new(
+                Expr::column([SET_TRANSACTION_NAME, "appId"]).is_not_null(),


I prefer safer myself.

kernel/src/expressions/column_names.rs

nicklan · 2024-10-29T21:42:25Z

kernel/src/expressions/column_names.rs

+/// square brackets may contain arbitrary characters, including periods and spaces. To include a
+/// literal square bracket in a field name, escape it by doubling it, e.g. `"[foo]]bar]"` would
+/// parse as `foo]bar`.
+// TODO: Marked as test-only for now, as a demonstration. Should we officially support it?


same comment re [[ as above

kernel/src/expressions/column_names.rs

nicklan

lgtm! seems there's an issue in the asan stuff in expression visiting though. maybe a non-deterministic leak?

scovich · 2024-10-30T22:46:23Z

seems there's an issue in the asan stuff in expression visiting though. maybe a non-deterministic leak?

Naturally it doesn't repro on my laptop... but I see it's a use-after-free on a strndup call:

SUMMARY: AddressSanitizer: heap-use-after-free ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:414 in __interceptor_strndup

and there's only one strndup invocation:

// utility to turn a slice into a char*   
char* allocate_string(const KernelStringSlice slice) {
  return strndup(slice.ptr, slice.len);
}

Most likely this is a lurking issue with kernel string slice lifetimes.
Can we prioritize merging #441 and see if that fixes it?

scovich · 2024-10-30T23:40:58Z

Most likely this is a lurking issue with kernel string slice lifetimes.
Can we prioritize merging #441 and see if that fixes it?

It worked!

scovich · 2024-10-31T02:08:54Z

kernel/src/scan/mod.rs

 pub enum ColumnType {
    // A column, selected from the data, as is
-    Selected(ColumnName),


@nicklan -- I believe this was a mistake of mine in the prefactor PR -- ColumnType intentionally tracks only top-level columns. So there's never a need for nesting and a normal String field name suffices.

zachschuermann

wow that was some fun parsing code :) just a couple comments

zachschuermann · 2024-11-05T23:51:53Z

kernel/src/expressions/column_names.rs

+    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+        let mut delim = None;
+        for s in self.iter() {
+            use std::fmt::Write as _;


any reason we can't use std::fmt::Write?

We could. The as _ indicates that we never directly refer to the trait -- we just need its methods to be available.

zachschuermann · 2024-11-05T23:57:06Z

kernel/src/expressions/column_names.rs

+type Chars<'a> = std::iter::Peekable<std::str::Chars<'a>>;
+
+/// Helper for `impl FromStr for ColumnName`.
+fn parse_simple(chars: &mut Chars<'_>) -> DeltaResult<(String, bool)> {


docs for bool return value?

zachschuermann · 2024-11-06T00:01:23Z

kernel/src/scan/data_skipping.rs

@@ -195,9 +195,11 @@ impl DataSkippingFilter {

        // Build the stats read schema by extracting the column names referenced by the predicate,
        // extracting the corresponding field from the table schema, and inserting that field.
+        //
+        // TODO: Support nested column names!


nicklan

still lgtm, one very minor nit

nicklan · 2024-11-06T18:39:36Z

kernel/src/expressions/column_names.rs

+    type Err = Error;
+
+    fn from_str(s: &str) -> DeltaResult<Self> {
+        // Ambiguous case: The empty string `""`could reasonably parse as either `ColumnName::new([""])`


nit, i think this comment belongs two lines lower

Will fix in the follow-up PR that adds support for parsing column lists, since this code will anyway move.

ColumnName tracks a sequence of field names instead of a simple string

baa75ca

scovich requested review from nicklan and OussamaSaoudi-db October 29, 2024 16:49

scovich commented Oct 29, 2024

View reviewed changes

ffi/src/expressions/kernel.rs Outdated Show resolved Hide resolved

fix use-after-free

2d9c0b8

nicklan reviewed Oct 29, 2024

View reviewed changes

kernel/src/expressions/column_names.rs Outdated Show resolved Hide resolved

small tweaks

cf2cbf2

nicklan approved these changes Oct 30, 2024

View reviewed changes

scovich added 4 commits October 30, 2024 15:28

double up '[' to match ']' for better readability

e00fb28

comment fix

6741b3a

fmt

ba1ad77

Merge remote-tracking branch 'oss/main' into column-name-paths

f3514df

scovich added 2 commits October 30, 2024 16:11

Support column name parsing

521035f

Merge remote-tracking branch 'oss/main' into column-name-paths

ff4c6f8

scovich commented Oct 31, 2024

View reviewed changes

zachschuermann self-requested a review November 5, 2024 18:18

zachschuermann approved these changes Nov 6, 2024

View reviewed changes

doc comments

1717de7

scovich mentioned this pull request Nov 6, 2024

parse table metadata.configuration as TableProperties #453

Merged

scovich added 2 commits November 6, 2024 10:32

use backticks instead of brackets; tolerate whitespace padding

9c4c2d3

fmt+clippy

9a22f5e

nicklan approved these changes Nov 6, 2024

View reviewed changes

zachschuermann approved these changes Nov 6, 2024

View reviewed changes

Merge branch 'main' into column-name-paths

b9c543e

scovich merged commit 4466509 into delta-io:main Nov 6, 2024
17 checks passed

scovich mentioned this pull request Nov 7, 2024

Choose an official Display format for ColumnName #444

Closed

scovich deleted the column-name-paths branch November 8, 2024 20:59

zachschuermann mentioned this pull request Dec 10, 2024

remove dependency on arrow-parquet ColumnPath #379

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ColumnName tracks a path of field names instead of a simple string #445

ColumnName tracks a path of field names instead of a simple string #445

scovich commented Oct 29, 2024 •

edited

Loading

codecov bot commented Oct 29, 2024 •

edited

Loading

scovich Oct 29, 2024

nicklan Oct 29, 2024

scovich Oct 29, 2024

scovich Oct 29, 2024

scovich Oct 29, 2024

nicklan Oct 29, 2024

scovich Oct 29, 2024

nicklan Oct 29, 2024

scovich Oct 29, 2024

scovich Oct 29, 2024

scovich Oct 29, 2024

nicklan Oct 29, 2024

nicklan left a comment

nicklan Oct 29, 2024

nicklan Oct 29, 2024

nicklan left a comment

scovich commented Oct 30, 2024 •

edited

Loading

scovich commented Oct 30, 2024

scovich Oct 31, 2024

zachschuermann left a comment

zachschuermann Nov 5, 2024

scovich Nov 6, 2024

zachschuermann Nov 5, 2024

zachschuermann Nov 6, 2024

nicklan left a comment

nicklan Nov 6, 2024

scovich Nov 6, 2024

		fn wrap_expression(state: &mut KernelExpressionVisitorState, expr: impl Into<Expression>) -> usize {
		state.inflight_expressions.insert(expr.into())

		// TODO: Marked as test-only for now, as a demonstration. Should this be Display for ColumnName, so
		// `ColumnName::new(["a", "b.c", "d"])` would print as `"a.[b.c].d"` instead of merely `"a.b.c.d"`?

ColumnName tracks a path of field names instead of a simple string #445

ColumnName tracks a path of field names instead of a simple string #445

Conversation

scovich commented Oct 29, 2024 • edited Loading

What changes are proposed in this pull request?

This PR affects the following public APIs

How was this change tested?

codecov bot commented Oct 29, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicklan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicklan left a comment

Choose a reason for hiding this comment

scovich commented Oct 30, 2024 • edited Loading

scovich commented Oct 30, 2024

Choose a reason for hiding this comment

zachschuermann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicklan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scovich commented Oct 29, 2024 •

edited

Loading

codecov bot commented Oct 29, 2024 •

edited

Loading

scovich commented Oct 30, 2024 •

edited

Loading