Skip to content

feat: make producer and consumer CLI arguments nicer #124

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.

Sign up for GitHub

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

Jump to bottom
Merged
ingomueller-net merged 2 commits into from
Nov 3, 2024
Merged

feat: make producer and consumer CLI arguments nicer #124

ingomueller-net merged 2 commits into from
Nov 3, 2024

Conversation

@ingomueller-net
Copy link
Contributor

@ingomueller-net ingomueller-net commented Nov 1, 2024

This PR changes the string that is given to select consumers and producers from being the respective class name to a shorter string. For example --producer=DuckDBProdcuer becomes --producer=duckdb.

@ingomueller-net
Copy link
Contributor Author

ingomueller-net commented Nov 1, 2024

I have run the commands in SUPPORT_MATRIX.md and they seem to work but I haven't tested the connection all the way to the web app, so there might be some more places that need to be adapted. With I bit of pointers, I might be able to test and change those as well.

@ingomueller-net
feat: make producer and consumer CLI arguments nicer
2df071f 
This PR changes the string that is given to select consumers and
producers from being the respective class name to a shorter string. For
example `--producer=DuckDBProdcuer` becomes `--producer=duckdb`.

Signed-off-by: Ingo Müller <ingomueller@google.com>
@richtia
Copy link
Member

richtia commented Nov 1, 2024

I have run the commands in SUPPORT_MATRIX.md and they seem to work but I haven't tested the connection all the way to the web app, so there might be some more places that need to be adapted. With I bit of pointers, I might be able to test and change those as well.

Thanks! Were you able to get a new producer_results.csv and consumer_results.csv after running those commands? If so, you can just update those in this PR as well

@ingomueller-net
chore: update CSV files
7f3a3bb 
Signed-off-by: Ingo Müller <ingomueller@google.com>
@ingomueller-net
Copy link
Contributor Author

ingomueller-net commented Nov 2, 2024

Thanks! Were you able to get a new producer_results.csv and consumer_results.csv after running those commands? If so, you can just update those in this PR as well

Done! I hadn't understood that those were tracked in the repo but I added them now.

It seems like quite a few results flipped from True to False. I guess that means that things that work before stopped working? I haven't investigated why that happens but I had looked into quite a few isolated cases before submitting and there didn't seem to be any breakage due to the changes. Still, I am not overall very confident, TBH...

@richtia
Copy link
Member

richtia commented Nov 2, 2024 •
edited
Loading

Thanks! Were you able to get a new producer_results.csv and consumer_results.csv after running those commands? If so, you can just update those in this PR as well

Done! I hadn't understood that those were tracked in the repo but I added them now.

It seems like quite a few results flipped from True to False. I guess that means that things that work before stopped working? I haven't investigated why that happens but I had looked into quite a few isolated cases before submitting and there didn't seem to be any breakage due to the changes. Still, I am not overall very confident, TBH...

Ibis has had a bunch of updates/issues since this was last run, so I can't really speak about those results. As for isthmus...i think I was previously passing all the skipped functions that weren't supported in the framework, now they are being skipped altogether. And skipped tests are marked as failed. Might be good to look more into this and possibly have a different status (instead of pass/fail) for unsupported functions for each engine in another PR?

@ingomueller-net ingomueller-net merged commit 723bf0f into substrait-io:main Nov 3, 2024
@ingomueller-net ingomueller-net deleted the pretty-cli-args branch November 3, 2024 10:29
This was referenced Nov 14, 2024
Closed
Merged
kou pushed a commit to apache/arrow that referenced this pull request Nov 15, 2024
@raulcd
GH-44726: [CI] Update substrait consumer call to use updated producer…
1afae0b 
… and consumer parameters (#44727)

### Rationale for this change

The nightly job for substrait integration is failing due to a change on the consumer-testing:
- substrait-io/consumer-testing#124

### What changes are included in this PR?

Update to new parameter.

### Are these changes tested?

Via archery

### Are there any user-facing changes?

No
* GitHub Issue: #44726

Authored-by: Raúl Cumplido <raulcumplido@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
QuietCraftsmanship pushed a commit to QuietCraftsmanship/arrow that referenced this pull request Jul 7, 2025
@raulcd
GH-44726: [CI] Update substrait consumer call to use updated producer…
9cb88b4 
… and consumer parameters (#44727)

### Rationale for this change

The nightly job for substrait integration is failing due to a change on the consumer-testing:
- substrait-io/consumer-testing#124

### What changes are included in this PR?

Update to new parameter.

### Are these changes tested?

Via archery

### Are there any user-facing changes?

No
* GitHub Issue: #44726

Authored-by: Raúl Cumplido <raulcumplido@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@richtia richtia richtia approved these changes

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ingomueller-net @richtia