-
Notifications
You must be signed in to change notification settings - Fork 564
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(SqlSmith): gen Table Subquery EXISTS
#4431
Conversation
There is 1 out of 1024 query that I run have this issue #4434 . Need to fix this first |
Codecov Report
@@ Coverage Diff @@
## main #4431 +/- ##
==========================================
- Coverage 73.86% 73.83% -0.03%
==========================================
Files 1002 1004 +2
Lines 162583 162749 +166
==========================================
+ Hits 120088 120166 +78
- Misses 42495 42583 +88
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
alright, I think it is nothing related to exists, we can merge it. Because the problem occur in subquery with aggregate. Probably this probability is very low, until now it is notice. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some exists
query, e.g.
SELECT t1.* FROM t1
WHERE EXISTS (SELECT * FROM t3 WHERE t1.c1 = t3.c1)
It can be transformed into a hash join, not necessarily a nested loop join.
SELECT t1.* FROM t1, t3 WHERE t1.c1 = t3.c1
So we can probably do more than this.
Clarification: Do you mean that exists can be transformed into something we already test (hash join), so we probably don't need to generate it, there are other more important things to generate? |
We still need to generate in these cases as we can test the There is a comment in this PR saying:
I thought it tries to say that because of this limitation, there is a set of queries we cannot generate in |
|
EXISTS
Currently a few feature not yet implemented
so I decided to workaround, as such With this, when I want to generate correlated subquery. The following problem shown up #6350 As for Imatz comment on the possible EXISTS subquery generated by materialised view. I will look into what I can do in a later time. |
The problem I facing is "Feature is not yet implemented: correlated subquery in HAVING or SELECT with agg," This show that if I would want to generate correlated query (Not the local query), I need to pass the information about "Cannot have agg" all the way until gen_expr which is too much of parameter passing. I am planning to refactor some code to able to workaround unsupported feature.
Previously, I didn't, store it as internal state because the function is keep on being called with in different context so to avoid mistake when I put them in internal state I didn't put them in internal state. But then now I feel it would be better to refactor this part to allow more workaround. Since this PR is not about correlated query, and it is about EXISTS, would it be alright if I dun generate correlated query (meaning I generate local context) and have another PR about generating correlated query. I need some advise on this as I think the refactor may need to change not less part. |
Sounds good. |
This is a good point, I think the way we workaround unsupported features have added complexity to the code. So sqlsmith will still generate these invalid queries, but will just ignore them. This is similar to our approach for permissible runtime errors: risingwave/src/tests/sqlsmith/src/runner.rs Lines 139 to 145 in 7a12a6b
Drawback is if sqlsmith generates too many of these. We can verify if this is the case by writing sqlsmith test to check if percentage of generated |
Lets continue discussion under this issue #6592 |
Hey @marvenlee2486, this pull request failed to merge and has been dequeued from the merge train. If you believe your PR failed in the merge train because of a flaky test, requeue it by clicking "Update branch" or pushing an empty commit with |
I hereby agree to the terms of the Singularity Data, Inc. Contributor License Agreement.
What's changed and what's your intention?
EXISTS implement
Checklist
./risedev check
(or alias,./risedev c
)Refer to a related PR or issue link (optional)
close #4296