You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2020/10/13 20:03:05 UTC
[GitHub] [beam] pawelpasterz opened a new pull request #13094: [BEAM-5570] Update javacc dependency
pawelpasterz opened a new pull request #13094:
URL: https://github.com/apache/beam/pull/13094
Update javacc dependency
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
- [ ] Format the pull request title like `[BEAM-XXX] Fixes bug in ApproximateQuantiles`, where you replace `BEAM-XXX` with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
Post-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
Lang | SDK | Dataflow | Flink | Samza | Spark | Twister2
--- | --- | --- | --- | --- | --- | ---
Go | [](https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/) | ---
Java | [](https://ci-beam.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Twister2/lastCompletedBuild/)
Python | [](https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python38/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python_PVR_Flink_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/) | ---
XLang | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Direct/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/) | ---
Pre-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
--- |Java | Python | Go | Website | Whitespace | Typescript
--- | --- | --- | --- | --- | --- | ---
Non-portable | [](https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocker_Cron/lastCompletedBuild/) <br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocs_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Whitespace_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Typescript_Cron/lastCompletedBuild/)
Portable | --- | [](https://ci-beam.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/) | --- | --- | --- | ---
See [.test-infra/jenkins/README](https://github.com/apache/beam/blob/master/.test-infra/jenkins/README.md) for trigger phrase, status and link of all Jenkins jobs.
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] pawelpasterz commented on pull request #13094: [BEAM-5570] Update javacc dependency
Posted by GitBox <gi...@apache.org>.
pawelpasterz commented on pull request #13094:
URL: https://github.com/apache/beam/pull/13094#issuecomment-708193886
R: @iemejia
R: @aromanenko-dev
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] amaliujia edited a comment on pull request #13094: [BEAM-5570] Update javacc dependency
Posted by GitBox <gi...@apache.org>.
amaliujia edited a comment on pull request #13094:
URL: https://github.com/apache/beam/pull/13094#issuecomment-717395399
I will support the idea that not to upgrade JavaCC for BeamSQL. The primary reason is Calcite pins itself at JavaCC 4.0 as well. So keeping JavaCC at 4.0 for BeamSQL will maintain compatibility.
Thus this PR LGTM
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] pawelpasterz commented on pull request #13094: [BEAM-5570] Update javacc dependency
Posted by GitBox <gi...@apache.org>.
pawelpasterz commented on pull request #13094:
URL: https://github.com/apache/beam/pull/13094#issuecomment-709786961
@iemejia @kanterov
I can see that after bumping up `javacc` in SQL the only one test failing is `BeamSqlDslAggregationTest#testUnsupportedDistinct`. It should fail due to incorrect distinct usage and actually, it is doing g so but `ParseException` message is a bit different. Here is the assertion:
```
java.lang.AssertionError:
Expected: (an instance of org.apache.beam.sdk.extensions.sql.impl.ParseException and exception with cause exception with message a string containing "Encountered \"*\"")
but: exception with cause exception with message a string containing "Encountered \"*\"" cause message was "Encountered "" at line 1, column 31.
```
https://gist.github.com/pawelpasterz/59900c7051baf76f073d3aad4984a7ed
As far as I was able to debug and figure it out on my own, I can say both cases reason for throwing `ParseException` is the same, just difference in message
And that is true for even small version change `4.0` -> `4.1`.
I am not even close to consider myself as beam's SQL module expert, tried to solve this issue on my own but failed to do so
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] amaliujia edited a comment on pull request #13094: [BEAM-5570] Update javacc dependency
Posted by GitBox <gi...@apache.org>.
amaliujia edited a comment on pull request #13094:
URL: https://github.com/apache/beam/pull/13094#issuecomment-717395399
I will support the idea that not to upgrade javacc for BeamSQL. The primary reason is Calcite pins itself at javacc 4.0 as well. So keeping javacc at 4.0 for BeamSQL will maintain compatibility.
Thus this PR LGTM
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] amaliujia merged pull request #13094: [BEAM-5570] Update javacc dependency
Posted by GitBox <gi...@apache.org>.
amaliujia merged pull request #13094:
URL: https://github.com/apache/beam/pull/13094
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] pawelpasterz commented on pull request #13094: [BEAM-5570] Update javacc dependency
Posted by GitBox <gi...@apache.org>.
pawelpasterz commented on pull request #13094:
URL: https://github.com/apache/beam/pull/13094#issuecomment-708293826
Run SQL PreCommit
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] kanterov commented on pull request #13094: [BEAM-5570] Update javacc dependency
Posted by GitBox <gi...@apache.org>.
kanterov commented on pull request #13094:
URL: https://github.com/apache/beam/pull/13094#issuecomment-708639097
@iemejia @pawelpasterz looks fine (for ClickHouse module), as soon as tests pass :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] amaliujia commented on pull request #13094: [BEAM-5570] Update javacc dependency
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #13094:
URL: https://github.com/apache/beam/pull/13094#issuecomment-718281226
@iemejia do you have any other comment on this PR?
I am planning to merge this PR in early next week?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] pawelpasterz commented on pull request #13094: [BEAM-5570] Update javacc dependency
Posted by GitBox <gi...@apache.org>.
pawelpasterz commented on pull request #13094:
URL: https://github.com/apache/beam/pull/13094#issuecomment-708172792
Run Java PreCommit
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] iemejia commented on pull request #13094: [BEAM-5570] Update javacc dependency
Posted by GitBox <gi...@apache.org>.
iemejia commented on pull request #13094:
URL: https://github.com/apache/beam/pull/13094#issuecomment-720479384
No, no issue here, it is a pity Calcite is so far behind, but that makes sense then.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] pawelpasterz closed pull request #13094: [BEAM-5570] Update javacc dependency
Posted by GitBox <gi...@apache.org>.
pawelpasterz closed pull request #13094:
URL: https://github.com/apache/beam/pull/13094
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] pawelpasterz edited a comment on pull request #13094: [BEAM-5570] Update javacc dependency
Posted by GitBox <gi...@apache.org>.
pawelpasterz edited a comment on pull request #13094:
URL: https://github.com/apache/beam/pull/13094#issuecomment-709786961
@iemejia @kanterov
I can see that after bumping up `javacc` in SQL the only one test failing is `BeamSqlDslAggregationTest#testUnsupportedDistinct`. It should fail due to incorrect distinct usage and actually, it is doing g so but `ParseException` message is a bit different. Here is the assertion:
```
java.lang.AssertionError:
Expected: (an instance of org.apache.beam.sdk.extensions.sql.impl.ParseException and exception with cause exception with message a string containing "Encountered \"*\"")
but: exception with cause exception with message a string containing "Encountered \"*\"" cause message was "Encountered "" at line 1, column 31.
```
https://gist.github.com/pawelpasterz/59900c7051baf76f073d3aad4984a7ed
As far as I was able to debug and figure it out on my own, I can say both cases reason for throwing `ParseException` is the same, just difference in message
And that is true for even small version change `4.0` -> `4.1`.
I am not even close to consider myself as beam's SQL module expert, tried to solve this issue on my own but failed to do so
Is it possible to update `javacc` just in clickhouse module?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] amaliujia edited a comment on pull request #13094: [BEAM-5570] Update javacc dependency
Posted by GitBox <gi...@apache.org>.
amaliujia edited a comment on pull request #13094:
URL: https://github.com/apache/beam/pull/13094#issuecomment-718281226
@iemejia do you have any other comment on this PR?
I am planning to merge this PR in early next week if there is no more comment.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] iemejia commented on pull request #13094: [BEAM-5570] Update javacc dependency
Posted by GitBox <gi...@apache.org>.
iemejia commented on pull request #13094:
URL: https://github.com/apache/beam/pull/13094#issuecomment-708226554
We also use javacc for Beam's SQL so worth to check the update there too.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] pawelpasterz commented on pull request #13094: [BEAM-5570] Update javacc dependency
Posted by GitBox <gi...@apache.org>.
pawelpasterz commented on pull request #13094:
URL: https://github.com/apache/beam/pull/13094#issuecomment-711816773
> One of the goals of update versions is to have the same version of the dependencies across Beam so let's try as possible to have them aligned.
>
> It looks like a more restrictive parsing exception, but still if it fails which is the 'good' expected result. Probably you can just adjust the message of expected exception in the test since is just this one.
After digging a bit deeper I think it's not only about adjusting expected message (unfortunately)
Closing this PR since this change requires more work
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] pawelpasterz commented on pull request #13094: [BEAM-5570] Update javacc dependency
Posted by GitBox <gi...@apache.org>.
pawelpasterz commented on pull request #13094:
URL: https://github.com/apache/beam/pull/13094#issuecomment-717063142
R: @amaliujia
R: @tysonjh
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] iemejia commented on pull request #13094: [BEAM-5570] Update javacc dependency
Posted by GitBox <gi...@apache.org>.
iemejia commented on pull request #13094:
URL: https://github.com/apache/beam/pull/13094#issuecomment-710136152
One of the goals of update versions is to have the same version of the dependencies across Beam so let's try as possible to have them aligned.
It looks like a more restrictive parsing exception, but still if it fails which is the 'good' expected result. Probably you can just adjust the message of expected exception in the test since is just this one.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] pawelpasterz removed a comment on pull request #13094: [BEAM-5570] Update javacc dependency
Posted by GitBox <gi...@apache.org>.
pawelpasterz removed a comment on pull request #13094:
URL: https://github.com/apache/beam/pull/13094#issuecomment-708010523
Run Java PreCommit
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] pawelpasterz commented on pull request #13094: [BEAM-5570] Update javacc dependency
Posted by GitBox <gi...@apache.org>.
pawelpasterz commented on pull request #13094:
URL: https://github.com/apache/beam/pull/13094#issuecomment-708153789
Run Java PreCommit
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] pawelpasterz commented on pull request #13094: [BEAM-5570] Update javacc dependency
Posted by GitBox <gi...@apache.org>.
pawelpasterz commented on pull request #13094:
URL: https://github.com/apache/beam/pull/13094#issuecomment-708348288
> We also use javacc for Beam's SQL so worth to check the update there too.
Looks like updating javacc in `extensions/sql` broke something, I will investigate
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] amaliujia commented on pull request #13094: [BEAM-5570] Update javacc dependency
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #13094:
URL: https://github.com/apache/beam/pull/13094#issuecomment-717395399
I will support the idea that to not upgrade JavaCC for BeamSQL. The primary reason is Calcite pins itself at JavaCC 4.0 as well. So keeping JavaCC at 4.0 for BeamSQL will maintain compatibility.
Thus this PR LGTM
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] pawelpasterz commented on pull request #13094: [BEAM-5570] Update javacc dependency
Posted by GitBox <gi...@apache.org>.
pawelpasterz commented on pull request #13094:
URL: https://github.com/apache/beam/pull/13094#issuecomment-708010523
Run Java PreCommit
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org