Skip to content

[rust] Validate append_arrow_batch nullability before enqueue - #3950

Open
charlesdong1991 wants to merge 1 commit into
apache:mainfrom
charlesdong1991:fix/arrow-batch-null-in-notnull
Open

[rust] Validate append_arrow_batch nullability before enqueue#3950
charlesdong1991 wants to merge 1 commit into
apache:mainfrom
charlesdong1991:fix/arrow-batch-null-in-notnull

Conversation

@charlesdong1991

Copy link
Copy Markdown
Contributor

Purpose

Linked issue: close #3949

Brief change log

append_arrow_batch can accept a caller-provided Arrow RecordBatch where a NOT NULL Fluss column contains null values. The write is accepted in some paths and the failure appears later during decode/scan.

Tests

  • add unit tests and integration tests


#[derive(Default)]
pub struct PrebuiltRecordBatchBuilder {
pub(crate) struct PrebuiltRecordBatchBuilder {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, i don't think this should be a public api in the first place

@charlesdong1991

Copy link
Copy Markdown
Contributor Author

cc @leekeiabstraction @fresh-borzoni PTAL

thanks in advance!! 🙏

@leekeiabstraction leekeiabstraction left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR. Left a comment

}

#[test]
fn prebuilt_builder_rejects_nulls_in_not_null_column() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add test cases for nested types?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i thought i left a comment that it doesn't support nested type because IIUC, java side doesn't work with nested types either based on code 🤔 i didn't have time to do an end-to-end run on Java yesterday because our PoC was for python...

i will check today and confirm if that is the case, and create an issue for Java if so...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[rust] append_arrow_batch accepts nulls in NOT NULL columns and fails later on scan

2 participants