Skip to content

[r] Push schema accessor down to libtiledbsoma#3079

Merged
johnkerl merged 9 commits intomainfrom
kerl/schema-pushdown
Oct 3, 2024
Merged

[r] Push schema accessor down to libtiledbsoma#3079
johnkerl merged 9 commits intomainfrom
kerl/schema-pushdown

Conversation

@johnkerl
Copy link
Copy Markdown
Contributor

@johnkerl johnkerl commented Sep 27, 2024

Issue and/or context: #2406 #3055 [sc-55682]

Changes:

Notes for Reviewer:

@johnkerl johnkerl force-pushed the kerl/schema-pushdown branch 4 times, most recently from 2998b20 to c0c329f Compare October 1, 2024 03:04
@johnkerl
Copy link
Copy Markdown
Contributor Author

johnkerl commented Oct 1, 2024 •
edited
Loading

Update as of 2024-09-30:

  • Fails unit test here
    • Old/new schema comparison fails in check_arrow_schema_data_types in R/utils-arrow.R: value-type binary vs value-type string
  • I've traced this down to this one line
    • TileDB-R's schema evolution (not yet ported to libtiledbsoma) writes value-type binary ([r] Use libtiledbsoma for schema evolution #3057)
    • Later on in the unit-test spot mentioned above that fails to check against value-type readback using the schema getter on this PR (which has been ported to libtiledbsoma)

Update 2024-10-03: resolved by #3100, which has been merged, and this has now rebased on top of.

@johnkerl johnkerl mentioned this pull request Oct 1, 2024
Open
@johnkerl johnkerl force-pushed the kerl/schema-pushdown branch from c0c329f to dbfc96f Compare October 1, 2024 03:12
@johnkerl johnkerl mentioned this pull request Oct 1, 2024
Open
@johnkerl johnkerl force-pushed the kerl/schema-pushdown branch from dbfc96f to 5e81f12 Compare October 2, 2024 17:45
@johnkerl johnkerl removed the blocked label Oct 2, 2024
@johnkerl johnkerl force-pushed the kerl/schema-pushdown branch from 5e81f12 to 3f1a3f0 Compare October 3, 2024 16:05
@johnkerl johnkerl marked this pull request as ready for review October 3, 2024 16:05
mojaveazure
mojaveazure approved these changes Oct 3, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@mojaveazure mojaveazure left a comment

Choose a reason for hiding this comment

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

LGTM; left one minor comment to future-proof the code. Please remember to bump the develop version, update the changelog, and 🚢

Comment thread apis/r/R/SOMADataFrame.R Outdated
@johnkerl johnkerl mentioned this pull request Oct 3, 2024
Merged
@johnkerl johnkerl force-pushed the kerl/schema-pushdown branch from 537e9ed to c98ceb6 Compare October 3, 2024 20:01
@johnkerl johnkerl merged commit c50cb98 into main Oct 3, 2024
@johnkerl johnkerl deleted the kerl/schema-pushdown branch October 3, 2024 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mojaveazure mojaveazure mojaveazure approved these changes

@nguyenv nguyenv Awaiting requested review from nguyenv

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@johnkerl @mojaveazure