feat!(ffi): support field nullability in schema visitor #656

kssenii · 2025-01-22T13:22:43Z

new feature PR: "feat: support for field nullability in ffi api"

What changes are proposed in this pull request?

Added support for field nullability in ffi api. It was not supported before, but it still important to know whether fields are nullable or not, because some applications might work more efficiently with non-nullable types.

This PR affects the following public APIs

New argument is_nullable is added to EngineSchemaVisitor visitor functions.

How was this change tested?

Tested on ClickHouse's delta lake related tests.

scovich

Thanks for tackling this -- it's a day one gap I'm surprised we got away with ignoring for so long.

ffi/src/schema.rs

scovich · 2025-01-22T16:44:34Z

ffi/src/schema.rs

@@ -143,21 +147,21 @@ pub unsafe extern "C" fn visit_schema(
    fn visit_struct_fields(visitor: &EngineSchemaVisitor, s: &StructType) -> usize {
        let child_list_id = (visitor.make_field_list)(visitor.data, s.fields.len());
        for field in s.fields() {
-            visit_schema_item(field.data_type(), field.name(), visitor, child_list_id);
+            visit_schema_item(field.data_type(), field.name(), visitor, child_list_id, field.is_nullable());


The visit_schema_item helper is private, so we should consider where to inject the new arg for maximum readability going forward -- probably not at end like this?

Honestly, the current arg order is already a bit weird, so maybe we should reorder the whole thing:

Suggested change

visit_schema_item(field.data_type(), field.name(), visitor, child_list_id, field.is_nullable());

visit_schema_item(field.name(), field.data_type(), field.is_nullable(), visitor, child_list_id);

yea I agree if it isn't too much trouble to just reorder the args like proposed here - it matches the typical parameter ordering that i'm aware of in Arrow, etc.

scovich · 2025-01-22T16:49:53Z

ffi/src/schema.rs

+                    // TODO: Should we no longer support value_contains_null argument in visitor_map
+                    // if nullability of the value will anyway be known through simple type visitor?
+                    // Same for contains_null in visitor_array.


Very good question. This is anyway a breaking change (because all the type visitors need to be updated with the new nullable arg. If we don't remove this now-redundant arg, it's just another thing that can potentially get out of sync as the code base evolves.

Actually... we still need a boolean nullability arg, but this PR changes its interpretation. Before this PR, it indicates whether the map contains nullable values. After this PR, it needs to indicate whether the map itself is nullable (e.g. because the map is a nullable struct field). Passing two booleans would just invite confusion IMO.

if we change the semantics of that boolean to "whether the map itself is nullable" we still retain the "whether the map contains nullable values" by leveraging the primitive's own nullability right?

Yes, as the TODO itself observes

if we change the semantics of that boolean to "whether the map itself is nullable" we still retain the "whether the map contains nullable values" by leveraging the primitive's own nullability right?

yes, that's what I meant, e.g. user can find out whether map value / array value is nullable not from visitor_map/visitor_array argument contains_null, but from its own visitor (whatever it is - visitor_string, etc). Therefore having map/array value nullability indication in two places is strange and ambiguous, so I'd remove it from visitor_map/visitor_array (and leave there only argument which indicates whether map/array itself is nullable).

yep sorry just me clarifying :)

scovich · 2025-01-22T16:55:23Z

ffi/src/schema.rs

+        is_nullable: bool,
        contains_null: bool, // if this array can contain null values


Suggested change

is_nullable: bool,

contains_null: bool, // if this array can contain null values

is_nullable: bool,

(see commentary about the TODO flagged below)

scovich · 2025-01-22T16:55:31Z

ffi/src/schema.rs

+        is_nullable: bool,
        value_contains_null: bool, // if this map can contain null values


Suggested change

is_nullable: bool,

value_contains_null: bool, // if this map can contain null values

is_nullable: bool,

(see commentary about the TODO flagged below)

scovich · 2025-01-22T16:56:02Z

ffi/src/schema.rs

        precision: u8,
        scale: u8,
    ),

    /// Visit a `string` belonging to the list identified by `sibling_list_id`.
    pub visit_string:
-        extern "C" fn(data: *mut c_void, sibling_list_id: usize, name: KernelStringSlice),
+        extern "C" fn(data: *mut c_void, sibling_list_id: usize, name: KernelStringSlice, is_nullable: bool),


Pretty sure fmt check will fail on this and similar long lines below?

@kssenii you can just run cargo fmt to get everything autoformatted nicely!

scovich · 2025-01-22T16:57:58Z

Not sure why semver didn't flag the callback signature change as breaking??

codecov · 2025-01-22T17:01:54Z

Codecov Report

Attention: Patch coverage is 0% with 36 lines in your changes missing coverage. Please review.

Project coverage is 83.92%. Comparing base (45eedf2) to head (44c58cf).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
ffi/src/schema.rs	0.00%	36 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #656      +/-   ##
==========================================
- Coverage   84.05%   83.92%   -0.13%     
==========================================
  Files          75       75              
  Lines       17251    17277      +26     
  Branches    17251    17277      +26     
==========================================
  Hits        14500    14500              
- Misses       2052     2078      +26     
  Partials      699      699

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

zachschuermann · 2025-01-22T17:01:07Z

ffi/examples/read-table/schema.h

@@ -2,6 +2,7 @@
 #include "read_table.h"
 #include "kernel_utils.h"
 #include <stdint.h>
+#include <time.h>


do we need the time header?

hmm I got this error otherwise

/home/kssenii/workspace/ClickHouse2/contrib/delta-kernel-rs/ffi/examples/read-table/read_table.c:168:24: error: implicit declaration of function ‘gmtime’ [-Wimplicit-function-declaration] 168 | struct tm *tm_info = gmtime(&tv.tv_sec); | ^~~~~~ /home/kssenii/workspace/ClickHouse2/contrib/delta-kernel-rs/ffi/examples/read-table/read_table.c:9:1: note: ‘gmtime’ is defined in header ‘<time.h>’; this is probably fixable by adding ‘#include <time.h>’ 8 | #include "schema.h" +++ |+#include <time.h> 9 | #include "kernel_utils.h"

(just noticed I should have added it in read_table.c instead of schema.h, but anyway that fixed the problem).
I will remove this header, would leave it for myself locally only.

zachschuermann · 2025-01-22T17:04:08Z

ffi/src/schema.rs

@@ -143,21 +147,21 @@ pub unsafe extern "C" fn visit_schema(
    fn visit_struct_fields(visitor: &EngineSchemaVisitor, s: &StructType) -> usize {
        let child_list_id = (visitor.make_field_list)(visitor.data, s.fields.len());
        for field in s.fields() {
-            visit_schema_item(field.data_type(), field.name(), visitor, child_list_id);
+            visit_schema_item(field.data_type(), field.name(), visitor, child_list_id, field.is_nullable());


yea I agree if it isn't too much trouble to just reorder the args like proposed here - it matches the typical parameter ordering that i'm aware of in Arrow, etc.

zachschuermann · 2025-01-22T17:05:01Z

ffi/src/schema.rs

        precision: u8,
        scale: u8,
    ),

    /// Visit a `string` belonging to the list identified by `sibling_list_id`.
    pub visit_string:
-        extern "C" fn(data: *mut c_void, sibling_list_id: usize, name: KernelStringSlice),
+        extern "C" fn(data: *mut c_void, sibling_list_id: usize, name: KernelStringSlice, is_nullable: bool),


@kssenii you can just run cargo fmt to get everything autoformatted nicely!

zachschuermann · 2025-01-22T17:08:07Z

ffi/src/schema.rs

+                    // TODO: Should we no longer support value_contains_null argument in visitor_map
+                    // if nullability of the value will anyway be known through simple type visitor?
+                    // Same for contains_null in visitor_array.


if we change the semantics of that boolean to "whether the map itself is nullable" we still retain the "whether the map contains nullable values" by leveraging the primitive's own nullability right?

…ay, because it is now redundant

scovich · 2025-01-22T18:05:45Z

ffi/src/schema.rs

+                call!(
+                    visit_array,
+                    visit_array_item(visitor, at, at.contains_null)
+                )


nit: more compact to factor out the nested function call:

Suggested change

call!(

visit_array,

visit_array_item(visitor, at, at.contains_null)

)

let child_list_id = visit_array_item(visitor, at, at.contains_null);

call!(visit_array, child_list_id)

Forgot to apply fmt one more time after f2fa5c5, so now it folded back to one line

scovich

LGTM!

scovich · 2025-01-22T19:45:33Z

nit: I think we need to tweak the PR title's prefix a tad? Maybe feat[ffi api]!: or something?
@zachschuermann ?

zachschuermann · 2025-01-22T20:01:30Z

nit: I think we need to tweak the PR title's prefix a tad? Maybe feat[ffi api]!: or something? @zachschuermann ?

yep thanks for flagging, editted

zachschuermann

LGTM just had a quick comment on removing those prints!

ffi/examples/read-table/schema.h

zachschuermann · 2025-01-22T22:39:40Z

ffi/examples/read-table/schema.h

  char* name_ptr = malloc(sizeof(char) * (name.len + 24));
  snprintf(name_ptr, name.len + 1, "%s", name.ptr);
-  snprintf(name_ptr + name.len, 24, " (contains null: %s)", contains_null ? "true" : "false");
+  snprintf(name_ptr + name.len, 24, " (is nullable: %s)", is_nullable ? "true" : "false");


I think we only have 22 byte max now? (it's either 21 or 22 bytes depending on if it's true/false)

scovich · 2025-01-23T15:44:42Z

@kssenii can you merge in latest main? There should be a button in the github UI, but I'm not the PR owner so I can't do it for you.

…lability

ffi: support field nullability

f3e8181

github-actions bot assigned kssenii Jan 22, 2025

kssenii mentioned this pull request Jan 22, 2025

ffi: support field nullability ClickHouse/delta-kernel-rs#1

Merged

scovich reviewed Jan 22, 2025

View reviewed changes

scovich added the breaking-change Change that will require a version bump label Jan 22, 2025

zachschuermann reviewed Jan 22, 2025

View reviewed changes

kssenii added 2 commits January 22, 2025 18:41

Apply cargo fmt; Better order of arguments in visit_schema_item

206299e

Remove value_contains_null/contains_null from visitor_map/visitor_arr…

f2fa5c5

…ay, because it is now redundant

scovich reviewed Jan 22, 2025

View reviewed changes

Update comment; apply fmt

1020389

scovich approved these changes Jan 22, 2025

View reviewed changes

zachschuermann changed the title ~~ffi api: support field nullability~~ feat![ffi]: support field nullability Jan 22, 2025

zachschuermann changed the title ~~feat![ffi]: support field nullability~~ feat![ffi]: support field nullability in schema visitor Jan 22, 2025

zachschuermann changed the title ~~feat![ffi]: support field nullability in schema visitor~~ feat!(ffi): support field nullability in schema visitor Jan 22, 2025

zachschuermann self-requested a review January 22, 2025 20:04

zachschuermann approved these changes Jan 22, 2025

View reviewed changes

ffi/examples/read-table/schema.h Show resolved Hide resolved

Return print

db56256

zachschuermann approved these changes Jan 22, 2025

View reviewed changes

kssenii added 2 commits January 23, 2025 11:51

Fix size

a333f2c

Fix size

6908d78

Merge remote-tracking branch 'origin/main' into ffi-support-field-nul…

44c58cf

…lability

zachschuermann merged commit d999b5c into delta-io:main Jan 23, 2025
19 of 21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!(ffi): support field nullability in schema visitor #656

feat!(ffi): support field nullability in schema visitor #656

kssenii commented Jan 22, 2025

scovich left a comment

scovich Jan 22, 2025

zachschuermann Jan 22, 2025

scovich Jan 22, 2025

scovich Jan 22, 2025

zachschuermann Jan 22, 2025

scovich Jan 22, 2025

kssenii Jan 22, 2025

zachschuermann Jan 22, 2025

scovich Jan 22, 2025

scovich Jan 22, 2025

scovich Jan 22, 2025

zachschuermann Jan 22, 2025

scovich commented Jan 22, 2025

codecov bot commented Jan 22, 2025 •

edited

Loading

zachschuermann Jan 22, 2025

kssenii Jan 22, 2025

zachschuermann Jan 22, 2025

zachschuermann Jan 22, 2025

zachschuermann Jan 22, 2025

scovich Jan 22, 2025

kssenii Jan 22, 2025

scovich left a comment

scovich commented Jan 22, 2025

zachschuermann commented Jan 22, 2025

zachschuermann left a comment

zachschuermann Jan 22, 2025

scovich commented Jan 23, 2025

	visit_schema_item(field.data_type(), field.name(), visitor, child_list_id, field.is_nullable());
	visit_schema_item(field.name(), field.data_type(), field.is_nullable(), visitor, child_list_id);

		is_nullable: bool,
		contains_null: bool, // if this array can contain null values

		is_nullable: bool,
		value_contains_null: bool, // if this map can contain null values

feat!(ffi): support field nullability in schema visitor #656

feat!(ffi): support field nullability in schema visitor #656

Conversation

kssenii commented Jan 22, 2025

What changes are proposed in this pull request?

This PR affects the following public APIs

How was this change tested?

scovich 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

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

scovich commented Jan 22, 2025

codecov bot commented Jan 22, 2025 • 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

scovich left a comment

Choose a reason for hiding this comment

scovich commented Jan 22, 2025

zachschuermann commented Jan 22, 2025

zachschuermann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scovich commented Jan 23, 2025

codecov bot commented Jan 22, 2025 •

edited

Loading