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.

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

fix(parquet): Add support for weak functions to register parquet readers and writers #11882

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kgpai
Copy link
Contributor

@kgpai kgpai commented Dec 16, 2024

Summary:
This diff marks the registerParquetfactory functions with the weak attribute. The weak attribute allows the linker to discard the weaker implementation of the registerParquetfactory functions and in place use the stronger one (See: https://en.wikipedia.org/wiki/Weak_symbol).
This should remove the need to compile the same files with different options, leading to ODRs when both targets are added as dependencies to the same binary.

Differential Revision: D67301778

…der/writers

Summary:
This diff marks the registerParquet*factory functions with the weak attribute. The weak attribute allows the linker to discard the weaker implementation of the registerParquet*factory functions and in place use the stronger one (See: https://en.wikipedia.org/wiki/Weak_symbol).
This should remove the need to compile the same files with different options,  leading to ODRs when both targets are added as dependencies to the same binary.

Differential Revision: D67301778
@kgpai kgpai requested a review from majetideepak as a code owner December 16, 2024 22:18
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 16, 2024
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D67301778

Copy link

netlify bot commented Dec 16, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit ffb483a
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/6760a7208595d80008b37e23

@kgpai kgpai changed the title fix(parquet) : Add support for weak functions to register parquet reader/writers fix(parquet) : Add support for weak functions to register parquet readers and writers Dec 16, 2024
@kgpai kgpai changed the title fix(parquet) : Add support for weak functions to register parquet readers and writers fix(parquet): Add support for weak functions to register parquet readers and writers Dec 16, 2024
@kgpai
Copy link
Contributor Author

kgpai commented Dec 16, 2024

cc: @majetideepak Some code cleanup so internally we dont have ODR violations.

@majetideepak
Copy link
Collaborator

@kgpai The same Wikipedia article mentions as such, inserting them into code is not very portable. Can you please share the issue you are seeing internally?

@majetideepak
Copy link
Collaborator

This should remove the need to compile the same files with different options When does this happen?

@kgpai
Copy link
Contributor Author

kgpai commented Dec 17, 2024

This should remove the need to compile the same files with different options When does this happen?

This happens internally - Not sure if you recall but we had to create two targets one with VELOX_ENABLE_PARQUET defined and one without it . The one with it defined is only used for tests and the other is used everywhere else where we have the registration code. This is problematic with our internal tools like automated linters which do not use the right dependency many times causing folks to link against the PARQUET enabled target and see linking errors or at worst runtime errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants