Skip to content

Merge all the things - #221

Merged
argmarco-tkd merged 16 commits into
mainfrom
merge_all_the_things
Dec 19, 2025
Merged

Merge all the things#221
argmarco-tkd merged 16 commits into
mainfrom
merge_all_the_things

Conversation

@sofia-tekdatum

Copy link
Copy Markdown

Merge dev_phase2 into main.

Main is up to date with Arrow release version 22.0

@github-actions

Copy link
Copy Markdown

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

See also:

@argmarco-tkd argmarco-tkd left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thank you for this. This is massive! (not a criticism, just a statement!)

Overall LGTM.

I only skimmed the cython code (unfamiliar with it), and the encryption/external code (assuming that's a copy/paste). For the rest, took a bit of a deeper look and did not find anything concerning - with one exception/note: for thrift auto-generated files, it did look like we downgraded the version of the thrift compiler. Left a note.

Left a couple of other minor comments around.

Thanks again!

Comment thread cpp/src/parquet/types.h
check(2547339, 0xffffffffffffffffULL);
}

TEST(TestIsParquetCipherSupported, SupportedCiphers) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

based on the types.cc file, I see that completeness of this is important. let's create a ticket to create an additional test (using magic enum, alredy imported into the project) to verify completeness.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

thanks.

Comment thread cpp/src/parquet/CMakeLists.txt
Comment thread cpp/src/parquet/encryption/encryption_utils.cc
Comment thread cpp/src/generated/parquet_types.h Outdated
Comment thread cpp/src/generated/parquet_types.cpp Outdated

@argmarco-tkd argmarco-tkd left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM - thank you! Ship it!

@argmarco-tkd
argmarco-tkd merged commit b63ada7 into main Dec 19, 2025
66 of 205 checks passed
@sofia-tekdatum
sofia-tekdatum deleted the merge_all_the_things branch January 24, 2026 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants