You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by GitBox <gi...@apache.org> on 2022/10/26 09:39:06 UTC
[GitHub] [shardingsphere] fang1025 opened a new pull request, #21768: fix ShardingSphereResultSet bug.
fang1025 opened a new pull request, #21768:
URL: https://github.com/apache/shardingsphere/pull/21768
Fixes bug.
Changes proposed in this pull request:
- ShorthandProjection.actualColumn just has columnProjection. when sql include ExpressionProjection(nvl.) and subquery，then ShardingSphereResultSet occur exception: Can not get index from column label "XX"
- ShorthandProjection add a property： resultSetColumns，when result set ，resultSetColumns to use instead of actualColumn.
- for example. this sql will occur error because of “nvl(LEAVE_DATE, '20991231') as leaveDate”:
> ```
> SELECT * FROM ( SELECT TMP_PAGE.*, ROWNUM ROW_ID FROM
> (select ID as id,NAME as name,nvl(LEAVE_DATE, '20991231') as leaveDate
> from STAFF_INFO) TMP_PAGE
> WHERE ROWNUM <= 10) WHERE ROW_ID > 0
> ```
---
Before committing this PR, I'm sure that I have checked the following options:
- [ ] My code follows the [code of conduct](https://shardingsphere.apache.org/community/en/involved/conduct/code/) of this project.
- [ ] I have self-reviewed the commit code.
- [ ] I have (or in comment I request) added corresponding labels for the pull request.
- [ ] I have passed maven check locally : `mvn clean install -B -T2C -DskipTests -Dmaven.javadoc.skip=true -e`.
- [ ] I have made corresponding changes to the documentation.
- [ ] I have added corresponding unit tests for my changes.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] fang1025 commented on a diff in pull request #21768: fix ShardingSphereResultSet bug.
Posted by GitBox <gi...@apache.org>.
fang1025 commented on code in PR #21768:
URL: https://github.com/apache/shardingsphere/pull/21768#discussion_r1014806861
##########
infra/binder/src/main/java/org/apache/shardingsphere/infra/binder/segment/select/projection/impl/ShorthandProjection.java:
##########
@@ -39,10 +39,17 @@ public final class ShorthandProjection implements Projection {
private final String owner;
private final Map<String, ColumnProjection> actualColumns = new LinkedHashMap<>();
-
- public ShorthandProjection(final String owner, final Collection<ColumnProjection> columnProjections) {
+
+ private final Map<String, Projection> resultSetColumns = new LinkedHashMap<>();
Review Comment:
> Can you merge actualColumns and resultSetColumns to one field?
In some cases, there just only need to use ColumnProjection. It may be better to keep a reference to actualColumns.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] fang1025 commented on a diff in pull request #21768: fix ShardingSphereResultSet bug.
Posted by GitBox <gi...@apache.org>.
fang1025 commented on code in PR #21768:
URL: https://github.com/apache/shardingsphere/pull/21768#discussion_r1012743257
##########
infra/binder/src/main/java/org/apache/shardingsphere/infra/binder/segment/select/projection/impl/ShorthandProjection.java:
##########
@@ -39,10 +39,17 @@ public final class ShorthandProjection implements Projection {
private final String owner;
private final Map<String, ColumnProjection> actualColumns = new LinkedHashMap<>();
-
- public ShorthandProjection(final String owner, final Collection<ColumnProjection> columnProjections) {
+
+ private final Map<String, Projection> resultSetColumns = new LinkedHashMap<>();
+
+ public ShorthandProjection(final String owner, final Collection<Projection> projections) {
this.owner = owner;
- columnProjections.forEach(each -> actualColumns.put(each.getExpression().toLowerCase(), each));
+ for (Projection projection : projections) {
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.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] strongduanmu commented on a diff in pull request #21768: fix ShardingSphereResultSet bug.
Posted by GitBox <gi...@apache.org>.
strongduanmu commented on code in PR #21768:
URL: https://github.com/apache/shardingsphere/pull/21768#discussion_r1012412338
##########
infra/binder/src/main/java/org/apache/shardingsphere/infra/binder/segment/select/projection/impl/ShorthandProjection.java:
##########
@@ -39,10 +39,17 @@ public final class ShorthandProjection implements Projection {
private final String owner;
private final Map<String, ColumnProjection> actualColumns = new LinkedHashMap<>();
-
- public ShorthandProjection(final String owner, final Collection<ColumnProjection> columnProjections) {
+
+ private final Map<String, Projection> resultSetColumns = new LinkedHashMap<>();
Review Comment:
Can you merge actualColumns and resultSetColumns to one field?
##########
infra/binder/src/main/java/org/apache/shardingsphere/infra/binder/segment/select/projection/impl/ShorthandProjection.java:
##########
@@ -39,10 +39,17 @@ public final class ShorthandProjection implements Projection {
private final String owner;
private final Map<String, ColumnProjection> actualColumns = new LinkedHashMap<>();
-
- public ShorthandProjection(final String owner, final Collection<ColumnProjection> columnProjections) {
+
+ private final Map<String, Projection> resultSetColumns = new LinkedHashMap<>();
+
+ public ShorthandProjection(final String owner, final Collection<Projection> projections) {
this.owner = owner;
- columnProjections.forEach(each -> actualColumns.put(each.getExpression().toLowerCase(), each));
+ for (Projection projection : projections) {
Review Comment:
Besides, please add unit test for these logic.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] strongduanmu merged pull request #21768: fix ShardingSphereResultSet bug.
Posted by GitBox <gi...@apache.org>.
strongduanmu merged PR #21768:
URL: https://github.com/apache/shardingsphere/pull/21768
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] fang1025 commented on a diff in pull request #21768: fix ShardingSphereResultSet bug.
Posted by GitBox <gi...@apache.org>.
fang1025 commented on code in PR #21768:
URL: https://github.com/apache/shardingsphere/pull/21768#discussion_r1014806861
##########
infra/binder/src/main/java/org/apache/shardingsphere/infra/binder/segment/select/projection/impl/ShorthandProjection.java:
##########
@@ -39,10 +39,17 @@ public final class ShorthandProjection implements Projection {
private final String owner;
private final Map<String, ColumnProjection> actualColumns = new LinkedHashMap<>();
-
- public ShorthandProjection(final String owner, final Collection<ColumnProjection> columnProjections) {
+
+ private final Map<String, Projection> resultSetColumns = new LinkedHashMap<>();
Review Comment:
> Can you merge actualColumns and resultSetColumns to one field?
In some cases, there just only need to use ColumnProjection. It may be better to keep a reference to actualColumns.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] fang1025 commented on a diff in pull request #21768: fix ShardingSphereResultSet bug.
Posted by GitBox <gi...@apache.org>.
fang1025 commented on code in PR #21768:
URL: https://github.com/apache/shardingsphere/pull/21768#discussion_r1015031901
##########
infra/binder/src/main/java/org/apache/shardingsphere/infra/binder/segment/select/projection/impl/ShorthandProjection.java:
##########
@@ -39,10 +39,17 @@ public final class ShorthandProjection implements Projection {
private final String owner;
private final Map<String, ColumnProjection> actualColumns = new LinkedHashMap<>();
-
- public ShorthandProjection(final String owner, final Collection<ColumnProjection> columnProjections) {
+
+ private final Map<String, Projection> resultSetColumns = new LinkedHashMap<>();
Review Comment:
> Can you merge actualColumns and resultSetColumns to one field?
It‘s done.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] codecov-commenter commented on pull request #21768: fix ShardingSphereResultSet bug.
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #21768:
URL: https://github.com/apache/shardingsphere/pull/21768#issuecomment-1292414490
# [Codecov](https://codecov.io/gh/apache/shardingsphere/pull/21768?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#21768](https://codecov.io/gh/apache/shardingsphere/pull/21768?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (efd44c5) into [master](https://codecov.io/gh/apache/shardingsphere/commit/32e4b3ab0e2cc3e4fd090491450455a29b3b49d6?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (32e4b3a) will **decrease** coverage by `0.00%`.
> The diff coverage is `69.56%`.
```diff
@@ Coverage Diff @@
## master #21768 +/- ##
============================================
- Coverage 60.80% 60.80% -0.01%
Complexity 2446 2446
============================================
Files 4118 4118
Lines 57440 57447 +7
Branches 9689 9692 +3
============================================
+ Hits 34926 34930 +4
- Misses 19611 19612 +1
- Partials 2903 2905 +2
```
| [Impacted Files](https://codecov.io/gh/apache/shardingsphere/pull/21768?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../segment/select/projection/ProjectionsContext.java](https://codecov.io/gh/apache/shardingsphere/pull/21768/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-aW5mcmEvYmluZGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9zaGFyZGluZ3NwaGVyZS9pbmZyYS9iaW5kZXIvc2VnbWVudC9zZWxlY3QvcHJvamVjdGlvbi9Qcm9qZWN0aW9uc0NvbnRleHQuamF2YQ==) | `96.29% <33.33%> (ø)` | |
| [...ent/select/projection/engine/ProjectionEngine.java](https://codecov.io/gh/apache/shardingsphere/pull/21768/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-aW5mcmEvYmluZGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9zaGFyZGluZ3NwaGVyZS9pbmZyYS9iaW5kZXIvc2VnbWVudC9zZWxlY3QvcHJvamVjdGlvbi9lbmdpbmUvUHJvamVjdGlvbkVuZ2luZS5qYXZh) | `93.54% <69.23%> (-2.06%)` | :arrow_down: |
| [...nt/select/projection/impl/ShorthandProjection.java](https://codecov.io/gh/apache/shardingsphere/pull/21768/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-aW5mcmEvYmluZGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9zaGFyZGluZ3NwaGVyZS9pbmZyYS9iaW5kZXIvc2VnbWVudC9zZWxlY3QvcHJvamVjdGlvbi9pbXBsL1Nob3J0aGFuZFByb2plY3Rpb24uamF2YQ==) | `92.85% <85.71%> (-7.15%)` | :arrow_down: |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] fang1025 commented on a diff in pull request #21768: fix ShardingSphereResultSet bug.
Posted by GitBox <gi...@apache.org>.
fang1025 commented on code in PR #21768:
URL: https://github.com/apache/shardingsphere/pull/21768#discussion_r1012742870
##########
infra/binder/src/main/java/org/apache/shardingsphere/infra/binder/segment/select/projection/impl/ShorthandProjection.java:
##########
@@ -39,10 +39,17 @@ public final class ShorthandProjection implements Projection {
private final String owner;
private final Map<String, ColumnProjection> actualColumns = new LinkedHashMap<>();
-
- public ShorthandProjection(final String owner, final Collection<ColumnProjection> columnProjections) {
+
+ private final Map<String, Projection> resultSetColumns = new LinkedHashMap<>();
Review Comment:
actualColumns is used in 'shardingsphere-encrypt-core' project.
I will confirm whether SHARDINGSPHERE-ENCRYPT-CORE can accept ExpressionProjection&ColumnProjection instead of ColumnProjection .
then I will reply to you
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org