-
Notifications
You must be signed in to change notification settings - Fork 4.5k
[BEAM-9379] vendor calcite 1.22.0 #11058
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
Conversation
|
As I recall, we can merge this PR first and then start a vendor calcite release, right? |
|
Run Gradle Build |
|
Please run the linkage checker to see if there were any changes. Optionally, reduce the set of linkage errors being reported. |
|
rerun |
Seem to be new warnings. Is the minidev JSON dependency important/optional/provided? |
|
yes, it is optional. I reduced the set of linkage errors being reported. |
|
retest this please |
|
The vendored jar fails validation. You can rerun the validation locally with Your shading without relocating lots of classes, such as: The above command will enumerate them all. |
|
looking these classes without relocating |
|
fixed and passed vendored jar validation |
|
retest this please |
|
Thank you @vectorijk. I will merge this PR after tests pass. |
|
retest this please |
vendor/calcite-1_22_0/build.gradle
Outdated
| "org.objectweb", | ||
| "org.objenesis", | ||
| "org.pentaho", | ||
| "org.slf4j", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you relocate logging, you will lose all logging from this vendored library.
Consider not relocating them and marking them as runtime depedencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move logging library as runtime dependencies
vendor/calcite-1_22_0/build.gradle
Outdated
| "org.testng:testng:7.1.0", | ||
| ], | ||
| runtimeDependencies: [ | ||
| "com.jayway.jsonpath:json-path:2.4.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you relocating com.jayway and marking json-path as a runtime dependency?
Ditto for other libraries below such as commons-logging, antlr, slf4j
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move antlr to dependencies and relocate it and not relocate commons-logging and slf4j
|
Since shading/vendoring seems need more knowledge to do it right, I will also try to test this PR once I have some time this week. |
|
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@beam.apache.org list. Thank you for your contributions. |
|
This pull request has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
|
Now there is a PR for a newer version of Calcite, we can work together there possible. #12962 |
vendor calcite 1.22.0
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.