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

ARROW-17066: [C++][Python][Substrait] "ignore_unknown_fields" should be specified when converting JSON to binary #13605

Merged
merged 8 commits into from Jul 22, 2022

Conversation

vibhatha
Copy link
Collaborator

@vibhatha vibhatha commented Jul 14, 2022

Substrait is continously changing and it introduces may unknown fields in the consumed plan. Since it is not practical to support all these fields simultaneously, we have to include a way to ignore such fields. To support that, the ignore_unknown_fields is added to the JsonParseOptions.

@vibhatha
Copy link
Collaborator Author

cc @westonpace

Not exactly sure if this is the expected fix. I may need some help to figure out a better test case too.
Please advise.

@github-actions
Copy link

@github-actions
Copy link

⚠️ Ticket has no components in JIRA, make sure you assign one.

@github-actions
Copy link

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

@vibhatha
Copy link
Collaborator Author

vibhatha commented Jul 14, 2022

Need to figure out a better way to represent substrait plans in strings.

@richtia
Copy link

richtia commented Jul 14, 2022

I'll try to test it out using your branch

@richtia
Copy link

richtia commented Jul 14, 2022

I tried your branch, but since it also has the latest substrait now, i'm actually hitting some other issues, that Weston will be fixing. ARROW-15582/ARROW-15538

@vibhatha vibhatha marked this pull request as ready for review July 15, 2022 02:55
python/pyarrow/tests/test_substrait.py Outdated Show resolved Hide resolved
@pitrou
Copy link
Member

pitrou commented Jul 21, 2022

@vibhatha Can you update the PR description since it will end up as the commit message?

@vibhatha
Copy link
Collaborator Author

@pitrou Sure I will update it.

@vibhatha
Copy link
Collaborator Author

@pitrou Updated the description.
@westonpace removed the todo comment.

@vibhatha vibhatha requested a review from westonpace July 21, 2022 15:41
@pitrou
Copy link
Member

pitrou commented Jul 21, 2022

@vibhatha Well, can you make the PR description actually descriptive, instead of merely repeting the PR title? Thanks :-)

@pitrou
Copy link
Member

pitrou commented Jul 21, 2022

i.e., explain why this is needed.

@vibhatha
Copy link
Collaborator Author

@vibhatha Well, can you make the PR description actually descriptive, instead of merely repeting the PR title? Thanks :-)

I will modify it 👍

@vibhatha
Copy link
Collaborator Author

@pitrou could you please check the updated description? Is it okay?

@pitrou pitrou merged commit 791e5bd into apache:master Jul 22, 2022
@ursabot
Copy link

ursabot commented Jul 23, 2022

Benchmark runs are scheduled for baseline = 32016b1 and contender = 791e5bd. 791e5bd is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Failed ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.1% ⬆️0.0%] test-mac-arm
[Failed ⬇️0.32% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.25% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Failed] 791e5bd6 ec2-t3-xlarge-us-east-2
[Failed] 791e5bd6 test-mac-arm
[Failed] 791e5bd6 ursa-i9-9960x
[Finished] 791e5bd6 ursa-thinkcentre-m75q
[Failed] 32016b1a ec2-t3-xlarge-us-east-2
[Failed] 32016b1a test-mac-arm
[Failed] 32016b1a ursa-i9-9960x
[Finished] 32016b1a ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

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.

None yet

5 participants