Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

GH-37983: [JS] Allow nullable fields in table when constructed from vector with nulls #39254

Merged
merged 1 commit into from
Dec 17, 2023
Merged

GH-37983: [JS] Allow nullable fields in table when constructed from vector with nulls #39254

merged 1 commit into from
Dec 17, 2023

Conversation

domoritz
Copy link
Member

@domoritz domoritz commented Dec 16, 2023 •
edited
Loading

We check whether a vector has nulls and then create the schema accordingly.

@domoritz domoritz requested a review from trxcllnt as a code owner December 16, 2023 20:00
@github-actions github-actions bot added awaiting committer review Awaiting committer review Component: JavaScript and removed awaiting committer review Awaiting committer review labels Dec 16, 2023
@domoritz domoritz mentioned this pull request Dec 16, 2023
Merged
bmschmidt
bmschmidt approved these changes Dec 17, 2023
View reviewed changes
Copy link
Contributor

@bmschmidt bmschmidt left a comment

Choose a reason for hiding this comment

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

LGTM.

@github-actions github-actions bot added the awaiting committer review Awaiting committer review label Dec 17, 2023
@domoritz domoritz merged commit e43f575 into apache:main Dec 17, 2023
@conbench-apache-arrow conbench-apache-arrow
Copy link

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit e43f575.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them.

@trxcllnt
Copy link
Contributor

trxcllnt commented Dec 27, 2023 •
edited
Loading

@domoritz the nullCount property is lazily computed, so accessing like this will materialize it for each Vector. This seems like a potentially huge performance hit? The Data class.has a nullable getter, so maybe we should promote that to the Vector class and use it instead?

@domoritz
Copy link
Member Author

Oh, yes, that would be much better. I was actually surprised that vectors don't have a nullable property.

domoritz added a commit that referenced this pull request Jan 3, 2024
@domoritz
GH-39255: [JS] Allow customization of schema when passing vectors to …
3d1324e 
…table constructor (#39256)

Merge after #39254.

* Closes: #39255
@domoritz domoritz mentioned this pull request Jan 3, 2024
Closed
@domoritz
Copy link
Member Author

domoritz commented Jan 3, 2024

Will do in #39435

@domoritz domoritz deleted the dom/nullable-table branch January 3, 2024 09:21
clayburn pushed a commit to clayburn/arrow that referenced this pull request Jan 23, 2024
@domoritz @clayburn
apacheGH-37983: [JS] Allow nullable fields in table when constructed …
b1fb6e5 
…from vector with nulls (apache#39254)
clayburn pushed a commit to clayburn/arrow that referenced this pull request Jan 23, 2024
@domoritz @clayburn
apacheGH-39255: [JS] Allow customization of schema when passing vecto…
af22ebf 
…rs to table constructor (apache#39256)

Merge after apache#39254.

* Closes: apache#39255
dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
@domoritz @dgreiss
apacheGH-37983: [JS] Allow nullable fields in table when constructed …
c023d5c 
…from vector with nulls (apache#39254)
dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
@domoritz @dgreiss
apacheGH-39255: [JS] Allow customization of schema when passing vecto…
ed0f4c2 
…rs to table constructor (apache#39256)

Merge after apache#39254.

* Closes: apache#39255
zanmato1984 pushed a commit to zanmato1984/arrow that referenced this pull request Feb 28, 2024
@domoritz @zanmato1984
apacheGH-39255: [JS] Allow customization of schema when passing vecto…
c7bc6e7 
…rs to table constructor (apache#39256)

Merge after apache#39254.

* Closes: apache#39255
@domoritz domoritz added this to the 15.0.0 milestone Mar 7, 2024
@domoritz domoritz mentioned this pull request Mar 27, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bmschmidt bmschmidt bmschmidt approved these changes

@trxcllnt trxcllnt Awaiting requested review from trxcllnt trxcllnt is a code owner

Assignees
No one assigned
Labels
awaiting committer review Awaiting committer review Component: JavaScript
Projects
None yet
Milestone
15.0.0
Development

Successfully merging this pull request may close these issues.

[JS] when creating a Table from Vectors, arrow always infers non-nullable Fields
3 participants
@domoritz @trxcllnt @bmschmidt