You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kylin.apache.org by GitBox <gi...@apache.org> on 2021/04/11 10:29:11 UTC
[GitHub] [kylin] zheniantoushipashi opened a new pull request #1626: [KYLIN-4554] validate filter condition on model saving
zheniantoushipashi opened a new pull request #1626:
URL: https://github.com/apache/kylin/pull/1626
verify filter condition using the following login
1 select * from tableName where {filterCondition} pass calcite parse
2 all column in filter condition must be model table column
## Proposed changes
Describe the big picture of your changes here to communicate to the maintainers why we should accept this pull request. If it fixes a bug or resolves a feature request, be sure to link to that issue.
## Types of changes
What types of changes does your code introduce to Kylin?
_Put an `x` in the boxes that apply_
- [ ] Bugfix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [ ] Documentation Update (if none of the other choices apply)
## Checklist
_Put an `x` in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code._
- [ ] I have create an issue on [Kylin's jira](https://issues.apache.org/jira/browse/KYLIN), and have described the bug/feature there in detail
- [ ] Commit messages in my PR start with the related jira ID, like "KYLIN-0000 Make Kylin project open-source"
- [ ] Compiling and unit tests pass locally with my changes
- [ ] I have added tests that prove my fix is effective or that my feature works
- [ ] If this change need a document change, I will prepare another pr against the `document` branch
- [ ] Any dependent changes have been merged
## Further comments
If this is a relatively large or complex change, kick off the discussion at user@kylin or dev@kylin by explaining why you chose the solution you did and what alternatives you considered, etc...
--
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] [kylin] coveralls edited a comment on pull request #1626: [KYLIN-4554] validate filter condition on model saving
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #1626:
URL: https://github.com/apache/kylin/pull/1626#issuecomment-817287678
## Pull Request Test Coverage Report for [Build 7076](https://coveralls.io/builds/38802117)
* **0** of **60** **(0.0%)** changed or added relevant lines in **3** files are covered.
* **6** unchanged lines in **2** files lost coverage.
* Overall coverage decreased (**-0.02%**) to **28.076%**
---
| Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
| :-----|--------------|--------|---: |
| [core-job/src/main/java/org/apache/kylin/job/JoinedFormatter.java](https://coveralls.io/builds/38802117/source?filename=core-job%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fkylin%2Fjob%2FJoinedFormatter.java#L59) | 0 | 6 | 0.0%
| [server-base/src/main/java/org/apache/kylin/rest/service/ModelService.java](https://coveralls.io/builds/38802117/source?filename=server-base%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fkylin%2Frest%2Fservice%2FModelService.java#L189) | 0 | 7 | 0.0%
| [core-metadata/src/main/java/org/apache/kylin/metadata/util/ModelUtil.java](https://coveralls.io/builds/38802117/source?filename=core-metadata%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fkylin%2Fmetadata%2Futil%2FModelUtil.java#L40) | 0 | 47 | 0.0%
<!-- | **Total:** | **0** | **60** | **0.0%** | -->
| Files with Coverage Reduction | New Missed Lines | % |
| :-----|--------------|--: |
| [server-base/src/main/java/org/apache/kylin/rest/util/QueryRequestLimits.java](https://coveralls.io/builds/38802117/source?filename=server-base%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fkylin%2Frest%2Futil%2FQueryRequestLimits.java#L72) | 1 | 47.62% |
| [core-metrics/src/main/java/org/apache/kylin/metrics/lib/impl/MetricsSystem.java](https://coveralls.io/builds/38802117/source?filename=core-metrics%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fkylin%2Fmetrics%2Flib%2Fimpl%2FMetricsSystem.java#L46) | 5 | 66.67% |
<!-- | **Total:** | **6** | | -->
| Totals | [](https://coveralls.io/builds/38802117) |
| :-- | --: |
| Change from base [Build 7067](https://coveralls.io/builds/38780467): | -0.02% |
| Covered Lines: | 26715 |
| Relevant Lines: | 95151 |
---
##### 💛 - [Coveralls](https://coveralls.io)
--
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] [kylin] coveralls commented on pull request #1626: [KYLIN-4554] validate filter condition on model saving
Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #1626:
URL: https://github.com/apache/kylin/pull/1626#issuecomment-817287678
## Pull Request Test Coverage Report for [Build 7050](https://coveralls.io/builds/38707373)
* **0** of **40** **(0.0%)** changed or added relevant lines in **2** files are covered.
* **7** unchanged lines in **2** files lost coverage.
* Overall coverage decreased (**-0.01%**) to **28.078%**
---
| Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
| :-----|--------------|--------|---: |
| [server-base/src/main/java/org/apache/kylin/rest/service/ModelService.java](https://coveralls.io/builds/38707373/source?filename=server-base%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fkylin%2Frest%2Fservice%2FModelService.java#L188) | 0 | 5 | 0.0%
| [core-metadata/src/main/java/org/apache/kylin/metadata/util/ModelUtil.java](https://coveralls.io/builds/38707373/source?filename=core-metadata%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fkylin%2Fmetadata%2Futil%2FModelUtil.java#L37) | 0 | 35 | 0.0%
<!-- | **Total:** | **0** | **40** | **0.0%** | -->
| Files with Coverage Reduction | New Missed Lines | % |
| :-----|--------------|--: |
| [tool/src/main/java/org/apache/kylin/tool/query/ProbabilityGenerator.java](https://coveralls.io/builds/38707373/source?filename=tool%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fkylin%2Ftool%2Fquery%2FProbabilityGenerator.java#L51) | 2 | 76.32% |
| [core-metrics/src/main/java/org/apache/kylin/metrics/lib/impl/MetricsSystem.java](https://coveralls.io/builds/38707373/source?filename=core-metrics%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fkylin%2Fmetrics%2Flib%2Fimpl%2FMetricsSystem.java#L46) | 5 | 66.67% |
<!-- | **Total:** | **7** | | -->
| Totals | [](https://coveralls.io/builds/38707373) |
| :-- | --: |
| Change from base [Build 7032](https://coveralls.io/builds/38574273): | -0.01% |
| Covered Lines: | 26711 |
| Relevant Lines: | 95131 |
---
##### 💛 - [Coveralls](https://coveralls.io)
--
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] [kylin] hit-lacus merged pull request #1626: [KYLIN-4554] validate filter condition on model saving
Posted by GitBox <gi...@apache.org>.
hit-lacus merged pull request #1626:
URL: https://github.com/apache/kylin/pull/1626
--
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] [kylin] hit-lacus edited a comment on pull request #1626: [KYLIN-4554] validate filter condition on model saving
Posted by GitBox <gi...@apache.org>.
hit-lacus edited a comment on pull request #1626:
URL: https://github.com/apache/kylin/pull/1626#issuecomment-817456513
Did you patch consider dynamic variable case? Like `date_str>='${start_date}' and date_str<'${end_date}'` .
--
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] [kylin] hit-lacus edited a comment on pull request #1626: [KYLIN-4554] validate filter condition on model saving
Posted by GitBox <gi...@apache.org>.
hit-lacus edited a comment on pull request #1626:
URL: https://github.com/apache/kylin/pull/1626#issuecomment-820317631
Since that the version 3.1.2 is going to be released, I wouldn't merge any new code change at the moment. I will merge it after 3.1.2 is released.
Thanks for your first contribution, @zheniantoushipashi , please go ahead!
--
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] [kylin] hit-lacus commented on pull request #1626: [KYLIN-4554] validate filter condition on model saving
Posted by GitBox <gi...@apache.org>.
hit-lacus commented on pull request #1626:
URL: https://github.com/apache/kylin/pull/1626#issuecomment-818403516
Besides, I think columns in dimension table should also be taken into account.
--
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] [kylin] hit-lacus commented on pull request #1626: [KYLIN-4554] validate filter condition on model saving
Posted by GitBox <gi...@apache.org>.
hit-lacus commented on pull request #1626:
URL: https://github.com/apache/kylin/pull/1626#issuecomment-817456513
Did you patch consider dynamic variable case?
--
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] [kylin] zheniantoushipashi commented on pull request #1626: [KYLIN-4554] validate filter condition on model saving
Posted by GitBox <gi...@apache.org>.
zheniantoushipashi commented on pull request #1626:
URL: https://github.com/apache/kylin/pull/1626#issuecomment-817631218
> Did you patch consider dynamic variable case? Like `date_str>='${start_date}' and date_str<'${end_date}'` .
>
> 
thanks for reminding， When saving the model, use the start_date and end_date of a mock to replace sql, and then verify
see the below test evidence
--
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] [kylin] zheniantoushipashi commented on pull request #1626: [KYLIN-4554] validate filter condition on model saving
Posted by GitBox <gi...@apache.org>.
zheniantoushipashi commented on pull request #1626:
URL: https://github.com/apache/kylin/pull/1626#issuecomment-817552441
**test evidence**
**Positive testing**
Use ssb data set， create a data model, add filter LO_CUSTKEY > 1 at last step
Successfully created the model
**Negative Testing**
Add an additional Y after the LO_CUSTKEY column to construct a column ：LO_CUSTKEYY that does not belong to the model table
The following error is reported when saving the model
Failed to deal with the request: java.lang.IllegalArgumentException: filter condition col: LO_CUSTKEYY is not a column in the table
Meet the test expectations
--
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] [kylin] zheniantoushipashi commented on pull request #1626: [KYLIN-4554] validate filter condition on model saving
Posted by GitBox <gi...@apache.org>.
zheniantoushipashi commented on pull request #1626:
URL: https://github.com/apache/kylin/pull/1626#issuecomment-819194725
I added all the columns of all tables to the validate collection, I did verification in ut and integration tests,
**test evidence**
--
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] [kylin] coveralls edited a comment on pull request #1626: [KYLIN-4554] validate filter condition on model saving
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #1626:
URL: https://github.com/apache/kylin/pull/1626#issuecomment-817287678
## Pull Request Test Coverage Report for [Build 7061](https://coveralls.io/builds/38755612)
* **0** of **46** **(0.0%)** changed or added relevant lines in **3** files are covered.
* **24** unchanged lines in **2** files lost coverage.
* Overall coverage decreased (**-0.02%**) to **28.07%**
---
| Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
| :-----|--------------|--------|---: |
| [core-job/src/main/java/org/apache/kylin/job/JoinedFormatter.java](https://coveralls.io/builds/38755612/source?filename=core-job%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fkylin%2Fjob%2FJoinedFormatter.java#L59) | 0 | 6 | 0.0%
| [server-base/src/main/java/org/apache/kylin/rest/service/ModelService.java](https://coveralls.io/builds/38755612/source?filename=server-base%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fkylin%2Frest%2Fservice%2FModelService.java#L189) | 0 | 6 | 0.0%
| [core-metadata/src/main/java/org/apache/kylin/metadata/util/ModelUtil.java](https://coveralls.io/builds/38755612/source?filename=core-metadata%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fkylin%2Fmetadata%2Futil%2FModelUtil.java#L36) | 0 | 34 | 0.0%
<!-- | **Total:** | **0** | **46** | **0.0%** | -->
| Files with Coverage Reduction | New Missed Lines | % |
| :-----|--------------|--: |
| [core-metrics/src/main/java/org/apache/kylin/metrics/lib/impl/MetricsSystem.java](https://coveralls.io/builds/38755612/source?filename=core-metrics%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fkylin%2Fmetrics%2Flib%2Fimpl%2FMetricsSystem.java#L48) | 8 | 61.67% |
| [engine-spark/src/main/java/org/apache/kylin/engine/spark/SparkCubingByLayer.java](https://coveralls.io/builds/38755612/source?filename=engine-spark%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fkylin%2Fengine%2Fspark%2FSparkCubingByLayer.java#L531) | 16 | 0% |
<!-- | **Total:** | **24** | | -->
| Totals | [](https://coveralls.io/builds/38755612) |
| :-- | --: |
| Change from base [Build 7060](https://coveralls.io/builds/38738752): | -0.02% |
| Covered Lines: | 26705 |
| Relevant Lines: | 95137 |
---
##### 💛 - [Coveralls](https://coveralls.io)
--
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] [kylin] hit-lacus commented on pull request #1626: [KYLIN-4554] validate filter condition on model saving
Posted by GitBox <gi...@apache.org>.
hit-lacus commented on pull request #1626:
URL: https://github.com/apache/kylin/pull/1626#issuecomment-820317631
Since that the version 3.1.2 is going to releasing, I would merge any new code change at the moment. I will merge it after 3.1.2 is released.
Thanks for your first contribution, @zheniantoushipashi please go ahead!
--
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] [kylin] zheniantoushipashi commented on pull request #1626: [KYLIN-4554] validate filter condition on model saving
Posted by GitBox <gi...@apache.org>.
zheniantoushipashi commented on pull request #1626:
URL: https://github.com/apache/kylin/pull/1626#issuecomment-818409750
retest this please
> Maybe we can make it more Robust?
>
> I change your UT a little, by changing from `TRANS_ID` to `kylin_account.TRANS_ID`, and find a mistake.
>
> ```java
> deserialize.setFilterCondition("kylin_account.TRANS_ID = 1");
> ```
>
> Following is the debug mode, it indicates that `allSqlIdentifier.add(id.names.get(0));` return `kylin_account`, not column name.
>
> 
>
> I think it could be better implemented, am I right ?
yes, you are right, should consider 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] [kylin] zheniantoushipashi commented on pull request #1626: [KYLIN-4554] validate filter condition on model saving
Posted by GitBox <gi...@apache.org>.
zheniantoushipashi commented on pull request #1626:
URL: https://github.com/apache/kylin/pull/1626#issuecomment-820321581
retest this please
--
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] [kylin] hit-lacus commented on pull request #1626: [KYLIN-4554] validate filter condition on model saving
Posted by GitBox <gi...@apache.org>.
hit-lacus commented on pull request #1626:
URL: https://github.com/apache/kylin/pull/1626#issuecomment-818400441
Maybe we can make it more Robust?
I change your UT a little, by changing from `TRANS_ID` to `kylin_account.TRANS_ID`, and find a mistake.
```java
deserialize.setFilterCondition("kylin_account.TRANS_ID = 1");
```
Following is the debug mode, it indicates that `allSqlIdentifier.add(id.names.get(0));` return `kylin_account`, not column name.
I think it could be better implemented, am I right ?
--
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