Skip to content

Add example for ScalarStructBuilder::new_null, fix display for nullScalarValue::Struct - #9238

Merged
alamb merged 4 commits into
apache:mainfrom
alamb:alamb/more_docs_about_struct
Feb 16, 2024
Merged

Add example for ScalarStructBuilder::new_null, fix display for nullScalarValue::Struct#9238
alamb merged 4 commits into
apache:mainfrom
alamb:alamb/more_docs_about_struct

Conversation

@alamb

@alambalamb commented Feb 15, 2024

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Follow on to #9229
related to #9227

Rationale for this change

@NGA-TRAN asked internally about exactly what ScalarStructBuilder::new_null internally so I wanted to document it in an example here.

When I did the PR the display didn't match what I thought new_null did, so I fixed a bug in null display as well

What changes are included in this PR?

  1. Add a doc example for ScalarStructBuilder::new_null
  2. Fix bug in ScalarValue::Struct null display
  3. Add test for ScalarValue::Struct display
  4. Fix a typo

Are these changes tested?

Yes, new test and by CI

Are there any user-facing changes?

@alambalamb changed the title Minor: Add example for ScalarStructBuilder::new_nullMinor: Add example for ScalarStructBuilder::new_null, fix display for nullScalarValue::StructFeb 15, 2024
@alambalamb changed the title Minor: Add example for ScalarStructBuilder::new_null, fix display for nullScalarValue::StructAdd example for ScalarStructBuilder::new_null, fix display for nullScalarValue::StructFeb 15, 2024
Comment on lines +44 to +46
/// Note this is different from a struct where each of the specified fields
/// are null (e.g. `{a: NULL}`)
///

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

@viiryaviirya left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me.

@alamb
alamb marked this pull request as ready for review February 16, 2024 10:21
// ScalarValue Struct should always have a single element
assert_eq!(struct_arr.len(), 1);

if struct_arr.null_count() == struct_arr.len() {

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This is the bug fix

let batch = RecordBatch::try_from_iter(vec![("s", arr as _)]).unwrap();

#[rustfmt::skip]
let expected = [

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This shows that new_null really does create a null value (and thus the change to impl Display is correct)

@alamb

Copy link
Copy Markdown
ContributorAuthor

FYI @jayzhan211

@jayzhan211jayzhan211 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.

LGTM

@alamb
alamb merged commit 14cb962 into apache:mainFeb 16, 2024
Sign up for freeto 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.

4 participants

@alamb@viirya@appletreeisyellow@jayzhan211