[BEAM-5570] Update javacc dependency

[pawelpasterz]

Update javacc dependency

---

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

• Choose reviewer(s) 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.

See the Contributor Guide for more tips on how to make review process smoother.

Post-Commit Tests Status (on master branch)

Lang	SDK	Dataflow	Flink	Samza	Spark	Twister2
Go		---		---		---
Java
Python				---		---
XLang		---		---		---

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website	Whitespace	Typescript
Non-portable
Portable	---		---	---	---	---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI.

[pawelpasterz]

Run Java PreCommit

[pawelpasterz]

Run Java PreCommit

[pawelpasterz]

R: @iemejia
R: @aromanenko-dev

[iemejia]

We also use javacc for Beam's SQL so worth to check the update there too.

[pawelpasterz]

Run SQL PreCommit

[pawelpasterz]

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

[kanterov]

@iemejia @pawelpasterz looks fine (for ClickHouse module), as soon as tests pass :)

[pawelpasterz]

@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?

[iemejia]

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.

[pawelpasterz]

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

[pawelpasterz]

R: @amaliujia
R: @tysonjh

[amaliujia]

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

[amaliujia]

@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.

[iemejia]

No, no issue here, it is a pity Calcite is so far behind, but that makes sense then.