Skip to content

Adding external dependencies to parquet/encryption/external/third_party - #111

Closed
argmarco-tkd wants to merge 2 commits into
dev_phase2from
dev_merge_p2_dll
Closed

Adding external dependencies to parquet/encryption/external/third_party#111
argmarco-tkd wants to merge 2 commits into
dev_phase2from
dev_merge_p2_dll

Conversation

@argmarco-tkd

@argmarco-tkd argmarco-tkd commented Sep 2, 2025

Copy link
Copy Markdown
Collaborator

Bringing in external dependencies (header files) into parquet/encryption/external/third_party

These will be used in a later PR. Additionally, these will eventually be moved out of this directory (#110)

This work is part of a series of PRs created to merge the "DLL Loading" work into dev_phase.
This work was already reviewed when "DLL Loading" (PR #37) was written against dev-miniapp.

@github-actions

github-actions Bot commented Sep 2, 2025

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:

@@ -0,0 +1,111 @@
//TODO: figure out the licensing.

#pragma once

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's use third_party instead of borrowed

Also there's only arrow/cpp/src/parquet/encryption/ on dev_phase2. Do we need external and also third_party nestings ?

@argmarco-tkd argmarco-tkd Sep 2, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Will modify to third_party.

Re: external: short answer is yes, we want it. This is the directory where we put DLL-and DBPA-related stuff (e.g. DLL loader). The ExternalDBPAEncryptorAdapter remains in parquet/encryption/external_dbpa_encryption.h.

So structure will be as followes

parquet/encryption/external_dbpa_encryption.h <- ExternalDBPAEncryptorAdapter
parquet/encryption/external/ <- DLL Loading and other related utils for DBPA loading/executing.
parquet/encryption/external/third_party/ <- header files, which eventually will go away #110)

@@ -0,0 +1,111 @@
//TODO: figure out the licensing.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's create an issue to track this. It may end up being related to the distribution tasks, but let's track it separately.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Already have one which covers all new code (#112)

#define DBPS_EXPORT
#endif

// TODO: this file was copied from

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think we have a tracking Issue for this. Can we add a reference to it?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

It's mentioned in the PR description. Will add a reference in the file as well.

@argmarco-tkd argmarco-tkd changed the title Adding external dependencies to external/borrowed/dbpa_interface.h Adding external dependencies to external/third_party/dbpa_interface.h Sep 2, 2025
@argmarco-tkd argmarco-tkd changed the title Adding external dependencies to external/third_party/dbpa_interface.h Adding external dependencies to parquet/encryption/external/third_party Sep 2, 2025
@argmarco-tkd

Copy link
Copy Markdown
Collaborator Author

Moved the files over to external/third_party - however that caused the specific file comments to disappear. They remain in the "conversation" part of the PR.

@avalerio-tkd avalerio-tkd left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM. Thanks for the updates.

@argmarco-tkd

Copy link
Copy Markdown
Collaborator Author

Merged as part of 1ce10de

@argmarco-tkd

Copy link
Copy Markdown
Collaborator Author

Added as part of 1ce10de

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