You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2020/06/14 05:45:35 UTC
[GitHub] [calcite] xy2953396112 opened a new pull request #2026: [CALCITE-4027] Add -Doverwrite option to DiffRepository
xy2953396112 opened a new pull request #2026:
URL: https://github.com/apache/calcite/pull/2026
https://issues.apache.org/jira/browse/CALCITE-4027
----------------------------------------------------------------
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] [calcite] hsyuan commented on pull request #2026: [CALCITE-4027] Add -Doverwrite option to DiffRepository
Posted by GitBox <gi...@apache.org>.
hsyuan commented on pull request #2026:
URL: https://github.com/apache/calcite/pull/2026#issuecomment-657941894
@xy2953396112 Can you update this pull request according to Julian's comments in the JIRA?
----------------------------------------------------------------
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] [calcite] vlsi commented on pull request #2026: [CALCITE-4027] Add -Doverwrite option to DiffRepository
Posted by GitBox <gi...@apache.org>.
vlsi commented on pull request #2026:
URL: https://github.com/apache/calcite/pull/2026#issuecomment-643723354
`overwrite` is a too broad option name.
Please ensure the option works from the command line, and it is documented as well.
----------------------------------------------------------------
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] [calcite] xy2953396112 edited a comment on pull request #2026: [WIP][CALCITE-4027] Add -Doverwrite option to DiffRepository
Posted by GitBox <gi...@apache.org>.
xy2953396112 edited a comment on pull request #2026:
URL: https://github.com/apache/calcite/pull/2026#issuecomment-658481050
> I made some review comments in https://issues.apache.org/jira/browse/CALCITE-4027. I don't think you've seen them.
Thank you very much for your comments.
----------------------------------------------------------------
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] [calcite] xy2953396112 commented on pull request #2026: [WIP][CALCITE-4027] Add -Doverwrite option to DiffRepository
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on pull request #2026:
URL: https://github.com/apache/calcite/pull/2026#issuecomment-658479917
> Am I doing it wrong?
> There is still error:
>
> ```
> error while creating xml parser
> java.lang.RuntimeException: error while creating xml parser
> at org.apache.calcite.test.DiffRepository.<init>(DiffRepository.java:219)
> at org.apache.calcite.test.DiffRepository.<init>(DiffRepository.java:131)
> at org.apache.calcite.test.DiffRepository$Key.toRepo(DiffRepository.java:833)
> at org.apache.calcite.test.DiffRepository.lookup(DiffRepository.java:770)
> at org.apache.calcite.test.DiffRepository.lookup(DiffRepository.java:734)
> at org.apache.calcite.test.DiffRepository.lookup(DiffRepository.java:720)
> at org.apache.calcite.test.RelOptRulesTest.getDiffRepos(RelOptRulesTest.java:229)
> at org.apache.calcite.test.RelOptTestBase.checkPlanning(RelOptTestBase.java:83)
> ```
>
> Here is the settings:
> 
Add -D `calcite.test.xml.overwrit` option in JUnit and run a test case in RelOptRulesTest.xml.
If we run a test case through Gradle, same as your error info, should fix it.
----------------------------------------------------------------
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] [calcite] xy2953396112 commented on a change in pull request #2026: [CALCITE-4027] Add -Doverwrite option to DiffRepository
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on a change in pull request #2026:
URL: https://github.com/apache/calcite/pull/2026#discussion_r440002413
##########
File path: core/src/test/java/org/apache/calcite/test/DiffRepository.java
##########
@@ -762,6 +764,20 @@ public static DiffRepository lookup(Class<?> clazz,
DiffRepository baseRepository,
Filter filter) {
final Key key = new Key(clazz, baseRepository, filter);
+ final String overwrite = System.getProperty("overwrite");
+ // Not need to add test cases manually when you add VM options.
+ // For example: -Doverwrite=true
+ if (overwrite != null && overwrite.equalsIgnoreCase("true")) {
+ try {
+ URL refFile = findFile(clazz, ".xml");
+ final String refFilePath = Sources.of(refFile).file().getAbsolutePath();
+ final String logFilePath = refFilePath.replace(".xml", "_actual.xml");
+ final File logFile = new File(logFilePath);
+ FileUtils.copyFile(logFile, new File(refFilePath.replace("out", "src")));
Review comment:
ok
----------------------------------------------------------------
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] [calcite] xy2953396112 commented on pull request #2026: [CALCITE-4027] Add -Doverwrite option to DiffRepository
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on pull request #2026:
URL: https://github.com/apache/calcite/pull/2026#issuecomment-643977129
@vlsi Thanks, How about `calcite.test.core.overwrite` ?
----------------------------------------------------------------
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] [calcite] hsyuan closed pull request #2026: [CALCITE-4027] Add -Doverwrite option to DiffRepository
Posted by GitBox <gi...@apache.org>.
hsyuan closed pull request #2026:
URL: https://github.com/apache/calcite/pull/2026
----------------------------------------------------------------
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] [calcite] hsyuan commented on pull request #2026: [CALCITE-4027] Add -Doverwrite option to DiffRepository
Posted by GitBox <gi...@apache.org>.
hsyuan commented on pull request #2026:
URL: https://github.com/apache/calcite/pull/2026#issuecomment-655277626
I am not questioning on the name. Can you show me how to update the expected xml file automatically if I added a test case into `RelOptRulesTest`?
----------------------------------------------------------------
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] [calcite] vlsi commented on pull request #2026: [CALCITE-4027] Add -Doverwrite option to DiffRepository
Posted by GitBox <gi...@apache.org>.
vlsi commented on pull request #2026:
URL: https://github.com/apache/calcite/pull/2026#issuecomment-643945214
@xy2953396112 , please have a look at https://github.com/apache/calcite/blob/978bb7ea44969351468d1b5e240e8f57af7e5770/core/src/main/java/org/apache/calcite/config/CalciteSystemProperty.java to get inspiration for the property name.
----------------------------------------------------------------
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] [calcite] hsyuan commented on a change in pull request #2026: [CALCITE-4027] Add -Doverwrite option to DiffRepository
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #2026:
URL: https://github.com/apache/calcite/pull/2026#discussion_r439835612
##########
File path: core/src/test/java/org/apache/calcite/test/DiffRepository.java
##########
@@ -762,6 +764,20 @@ public static DiffRepository lookup(Class<?> clazz,
DiffRepository baseRepository,
Filter filter) {
final Key key = new Key(clazz, baseRepository, filter);
+ final String overwrite = System.getProperty("overwrite");
+ // Not need to add test cases manually when you add VM options.
+ // For example: -Doverwrite=true
+ if (overwrite != null && overwrite.equalsIgnoreCase("true")) {
+ try {
+ URL refFile = findFile(clazz, ".xml");
+ final String refFilePath = Sources.of(refFile).file().getAbsolutePath();
+ final String logFilePath = refFilePath.replace(".xml", "_actual.xml");
+ final File logFile = new File(logFilePath);
+ FileUtils.copyFile(logFile, new File(refFilePath.replace("out", "src")));
Review comment:
Can we use guava `Files.copy`?
----------------------------------------------------------------
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] [calcite] xy2953396112 commented on pull request #2026: [CALCITE-4027] Add -Doverwrite option to DiffRepository
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on pull request #2026:
URL: https://github.com/apache/calcite/pull/2026#issuecomment-655505471
> I am not questioning on the name. Can you show me how to update the expected xml file automatically if I added a test case into `RelOptRulesTest`?
If we add a test case into `RelOptRulesTest`，we can add a VM options `-Dcalcite.test.xml.overwrite=true` in IDE and run this test case, and we can see the `RelOptRulesTest.xml` has been updated automatically. When we run the test again, it will 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] [calcite] xy2953396112 commented on pull request #2026: [CALCITE-4027] Add -Doverwrite option to DiffRepository
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on pull request #2026:
URL: https://github.com/apache/calcite/pull/2026#issuecomment-658043334
> @xy2953396112 Can you update this pull request according to Julian's comments in the JIRA?
OK, I'll update the code later.
----------------------------------------------------------------
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] [calcite] xy2953396112 commented on pull request #2026: [WIP][CALCITE-4027] Add -Doverwrite option to DiffRepository
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on pull request #2026:
URL: https://github.com/apache/calcite/pull/2026#issuecomment-658481050
> I made some review comments in https://issues.apache.org/jira/browse/CALCITE-4027. I don't think you've seen them.
Thank you very much for your 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] [calcite] xy2953396112 commented on pull request #2026: [CALCITE-4027] Add -Doverwrite option to DiffRepository
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on pull request #2026:
URL: https://github.com/apache/calcite/pull/2026#issuecomment-655276026
> @xy2953396112 With your patch, how do I update RelOptRulesTest expected file automatically?
> Say, I added a test in RelOptRulesTest, and specify the property in IDE, it doesn't update the expected file.
Thanks, I'm not sure if `calcite.test.xml.overwrite` is appropriate, maybe a better property name.
----------------------------------------------------------------
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] [calcite] hsyuan commented on pull request #2026: [CALCITE-4027] Add -Doverwrite option to DiffRepository
Posted by GitBox <gi...@apache.org>.
hsyuan commented on pull request #2026:
URL: https://github.com/apache/calcite/pull/2026#issuecomment-655584251
Am I doing it wrong?
There is still error:
```
error while creating xml parser
java.lang.RuntimeException: error while creating xml parser
at org.apache.calcite.test.DiffRepository.<init>(DiffRepository.java:219)
at org.apache.calcite.test.DiffRepository.<init>(DiffRepository.java:131)
at org.apache.calcite.test.DiffRepository$Key.toRepo(DiffRepository.java:833)
at org.apache.calcite.test.DiffRepository.lookup(DiffRepository.java:770)
at org.apache.calcite.test.DiffRepository.lookup(DiffRepository.java:734)
at org.apache.calcite.test.DiffRepository.lookup(DiffRepository.java:720)
at org.apache.calcite.test.RelOptRulesTest.getDiffRepos(RelOptRulesTest.java:229)
at org.apache.calcite.test.RelOptTestBase.checkPlanning(RelOptTestBase.java:83)
```
Here is the settings:
----------------------------------------------------------------
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