[ https://issues.apache.org/jira/browse/CALCITE-5909?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17752849#comment-17752849 ]
Ran Tao edited comment on CALCITE-5909 at 8/10/23 4:47 PM: ----------------------------------------------------------- [~thomas.rebele] thanks for suggestions. however, this problem is not the reason of ide, caching, multi-thread or data. I checked this problem carefully and in-depth. In fact, this case is wrong indeed. The reason why gradle build is normal is because this SqlParserTest was moved into the src/main directory in calcite-1.28, that is, it will not be executed as a unit test. But inheriting its subclasses such as BabelParserTest, ServerParserTest can execute it indirectly. But in calcite-1.28, for this case, it is restricted and cannot be executed in subclasses, that is, it cannot be included in the build or test phase in the end. Here are some important changes: *<= calcite-1.27.0:* SqlParserTest is in `core/test` dir (this case is right and can be covered by gradle build process) *calcite-1.28.0 ~ calcite-1.33.0:* move SqlParserTest to `testkit/main` dir (SqlParserTest not in test dir but it can be coverd by BabelParserTest, ServerParserTest indirectly. all cases in SqlParserTest run success except testNoUnintendedNewReservedKeywords, because this case has `not-run-for-subclasses` limitation, so it can't be covered in gradle build process, but this case is right.) *>= calcite-1.34.0:* this case is wrong, because the keywords are changed in these released version, but not update the static field of SqlParserTest.KEYWORDS. because of the reason above we can't found this failure in the build or ci process. The solution: 1. fix this case. I have opened a [PR|https://github.com/apache/calcite/pull/3361] to fix it. but [calcite-5564|https://issues.apache.org/jira/browse/CALCITE-5564] introduce PERCENTILE_CONT/PERCENTILE_DISC,to let it be NonReservedKeywords, however PERCENTILE_CONT/PERCENTILE_DISC is reserved keywords from sql-2008. it blocked my PR. 2.Only fix this case is not enough, it will be failed if we not cover it in calcite build process. I have try 2 ways. first way is re-move SqlParserTest to test directory, it's very difficult, because many src/main classes use it. If move it to test dir, these classes can not reference it. second way is remove the `not-run-for-subclasses` this limitation, it meets some other problems, however, it looks like can be solved (I hope get some feedbacks or contexts). Or maybe we can customize the test directory, it's a simple way. In terms of priority, I think PERCENTILE_CONT, PERCENTILE_DISC keywords need to be solved first (or let it be NonReservedKeywords?). [~julianhyde] If you have time, pls help to check it. was (Author: lemonjing): [~thomas.rebele] thanks for suggestions. however, this problem is not the reason of ide, caching, multi-thread or data. I checked this problem carefully and in-depth. In fact, this case is wrong indeed. The reason why gradle build is normal is because this SqlParserTest was moved into the src/main directory in calcite-1.28, that is, it will not be executed as a unit test. But inheriting its subclasses such as BabelParserTest, ServerParserTest can execute it indirectly. But in calcite-1.28, for this case, it is restricted and cannot be executed in subclasses, that is, it cannot be included in the build or test phase in the end. Here are some important changes: *<= calcite-1.27.0:* SqlParserTest is in `core/test` dir (this case is right and can be covered by gradle build process) *calcite-1.28.0 ~ calcite-1.33.0:* move SqlParserTest to `testkit/main` dir (SqlParserTest not in test dir but it can be coverd by BabelParserTest, ServerParserTest indirectly. all cases in SqlParserTest run success except testNoUnintendedNewReservedKeywords, because this case has `not-run-for-subclasses` limitation, so it can't be covered in gradle build process, but this case is right.) *>= calcite-1.34.0:* this case is wrong, because the keywords are changed in these released version, but not update the static field of SqlParserTest.KEYWORDS. because of the reason above we can't found this failure in the build or ci process. The solution: 1. fix this case. I have opened a [PR|https://github.com/apache/calcite/pull/3361] to fix it. but calcite-5564 introduce PERCENTILE_CONT/PERCENTILE_DISC,to let it be NonReservedKeywords, however PERCENTILE_CONT/PERCENTILE_DISC is reserved keywords from sql-2008. it blocked my PR. 2.Only fix this case is not enough, it will be failed if we not cover it in calcite build process. I have try 2 ways. first way is re-move SqlParserTest to test directory, it's very difficult, because many src/main classes use it. If move it to test dir, these classes can not reference it. second way is remove the `not-run-for-subclasses` this limitation, it meets some other problems, however, it looks like can be solved (I hope get some feedbacks or contexts). Or maybe we can customize the test directory, it's a simple way. In terms of priority, I think PERCENTILE_CONT, PERCENTILE_DISC keywords need to be solved first (or let it be NonReservedKeywords?). [~julianhyde] If you have time, pls help to check it. > Sometimes SqlParserTest.testNoUnintendedNewReservedKeywords fails in the IDE > but passes when run from the command line > ---------------------------------------------------------------------------------------------------------------------- > > Key: CALCITE-5909 > URL: https://issues.apache.org/jira/browse/CALCITE-5909 > Project: Calcite > Issue Type: Bug > Components: tests > Reporter: LakeShen > Priority: Minor > Labels: pull-request-available > Attachments: image-2023-08-08-23-32-55-466.png > > > When I run the SqlParserTest,the testNoUnintendedNewReservedKeywords method > failed,the exception like this: > {code:java} > java.lang.AssertionError: The parser has at least one new reserved keyword. > Are you sure it should be reserved? Difference: {code} > The picture like this: > !image-2023-08-08-23-32-55-466.png|width=1543,height=496! > I could fix this problem.More importantly, why is this method failing, but > the Calcite pipeline is passing? I think we should look at something we missed -- This message was sent by Atlassian Jira (v8.20.10#820010)