-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Exasol support #2046
Exasol support #2046
Conversation
Implement support of REGEXP_LIKE as LikeExpression as described here: https://docs.exasol.com/db/latest/sql_references/predicates/not_regexp_like.htm
Allow unparenthesesed sub selects being part of multiple function parameters
Thank you for your interest and contribution. I would look at it promptly, but please resolve the conflicts first (just resynchronize the repositories). |
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.
Please don't reformat files!
Beside this, there are 2 more relevant remarks.
src/main/java/net/sf/jsqlparser/expression/operators/relational/LikeExpression.java
Outdated
Show resolved
Hide resolved
@@ -4527,6 +4529,8 @@ Expression PrimaryExpression() #PrimaryExpression: | |||
|
|||
| LOOKAHEAD( ParenthesedSelect() , {!interrupted} ) retval=ParenthesedSelect() | |||
|
|||
| LOOKAHEAD( Select() , {!interrupted} ) retval=Select() |
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.
Please where is this coming from and why do we need it?
It's certainly misplaces and the Semantic Lookahead will make it terribly expensive.
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.
It is very possible that the two performance tests fail now because of this one.
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.
This comes (at least) from the need to support sub selects (without parentheses) as function arguments for functions with mutliple parameters, like the example implemented in the unit tests: SELECT COALESCE(SELECT mycolumn from mytable, 0)
From what I could see, this is a InternalFunction
and multiple parameters are handeled via ExpressionList
and ultimately handeled via PrimaryExpression
.
It's also likely that sub selects without parentheses are supported elsewhere (outside functions) in Exasol.
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.
Another example would be the following query, that won't be parsable without that change: select * from mytable where mycol = select mycol from anothertable
Greetings, two issues (beside the reformatting):
|
The choice conflicts are solved. Regarding the first one: I think this one does not come from my changes since the same error occurs on the master branch already. However, how can I reduce the timeout conditions of only those two tests? |
Please check my remark on the semantic lookahead for the But in general, yes you can increase the time out or reduce the number of loops for those 2 tests. Just check the source, it should be pretty self explaining. |
Are you referring to your comment regarding the |
yes.
where, I did not see any response to my remark? |
Directly above under your remarks, I've added multiple comments to the different topics. Unfortunately, I cannot link it since GitHub does not provide a link directly to those comments. |
Unfortunately I can't see any response, please don't ask me why. I am lost on this. Can you please explain it here in this discussion what this |
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.
I am really sorry, the remarks were still on pending and I didn't see how to publish them in the first look.
Hopefully, now you should see my comments. If not let me know and I'll post them as normal comments again.
src/main/java/net/sf/jsqlparser/expression/operators/relational/LikeExpression.java
Outdated
Show resolved
Hide resolved
@@ -4527,6 +4529,8 @@ Expression PrimaryExpression() #PrimaryExpression: | |||
|
|||
| LOOKAHEAD( ParenthesedSelect() , {!interrupted} ) retval=ParenthesedSelect() | |||
|
|||
| LOOKAHEAD( Select() , {!interrupted} ) retval=Select() |
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.
This comes (at least) from the need to support sub selects (without parentheses) as function arguments for functions with mutliple parameters, like the example implemented in the unit tests: SELECT COALESCE(SELECT mycolumn from mytable, 0)
From what I could see, this is a InternalFunction
and multiple parameters are handeled via ExpressionList
and ultimately handeled via PrimaryExpression
.
It's also likely that sub selects without parentheses are supported elsewhere (outside functions) in Exasol.
src/main/java/net/sf/jsqlparser/expression/operators/relational/LikeExpression.java
Outdated
Show resolved
Hide resolved
@@ -4527,6 +4529,8 @@ Expression PrimaryExpression() #PrimaryExpression: | |||
|
|||
| LOOKAHEAD( ParenthesedSelect() , {!interrupted} ) retval=ParenthesedSelect() | |||
|
|||
| LOOKAHEAD( Select() , {!interrupted} ) retval=Select() |
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.
Another example would be the following query, that won't be parsable without that change: select * from mytable where mycol = select mycol from anothertable
Ok, I can see the explanation now and understand what you try to achieve. However, it gives me some headache:
I am not opposing Exasol per se, but I do see Sub Selects without brackets as very evil: select select 1;
select 1 where 1 = select 1 + 1;
|
That's right, but only if it is the only parameter of the function, right?
But this works on Exasol.
You are right, I could implement that, but let us first discuss on how we proceed with that feature in general.
I agree, that this is evil and not really nice, but Exasol supports all of those statements. |
On formatting: you can always run the Spotless |
You are correct and I would prefer to extend the -- what exactly will this mean?
SELECT function(SELECT 1, 2)
Agreed. This matter is complex because it quickly can have unwanted negative side effects.
I believe you can understand for my example, why I have so many concerns here.
|
I tried this, but this reformats the whole file just as it has been in the first run. |
This is strange because only files with actual changes should be formatted. Sorry for the extra steps. |
Dialect specific parsing could make sense in other cases as well. Do you have any thoughts on how to implement it? Do you want to have it implemented over the whole grammar or just for this special case for now? |
No problem, will reformat the files. |
Yes, we just tried to avoid it so far since many features have an impact on many dialects.
I have thought about it for a long time actually and I see three ways in general: a) the Grammar is built from a template and we would end up with specific parsers
I feel like right know it should be an enum, which would cover only a very few really exceptional corner cases like So, in my opinion we should try 3 steps:
Lets see how far we get. |
Yes, exactly. |
Btw, 'IN' predicates seem to similar affected and its Exasol and DB2. |
The Exasol documentation is not helpful at all because it shows the brackets as mandatory: https://docs.exasol.com/db/latest/sql_references/predicates/not_in.htm |
I'll try to implement the steps above and agree with the last suggestion to call the feature something like |
Sounds like a plan. |
Sure, I will grant access to the fork. Thank you very much for your support, I really appreciate it. |
This looks really good to me, well done! Considering the |
Signed-off-by: Andreas Reichel <andreas@manticore-projects.com>
I have added a test for failing, when the specific flag is not set and this works as expected. Please decide on the best name for the feature (depending if we want to cover IN predicates also) and then we can merge. PS: really well done! |
Thank you for your support on this! I didn't really get the point yet on the DB2 IN predicate you mention. Isn't the whole syntax covered in the existing implementation? Maybe I miss something here or I have wrong documentations. Besides that, I think it is really related to unparenthesized sub selects or can you imagine on another expression as well? Before merging, I'd like to also cover the case, that an unparenthesized sub select can be a |
On the -- IN with one argument does not need brackets?!
select 1 from dual where a IN 1;
-- IN with two or more arguments needs brackets
select 1 from dual where a IN (1, 2); Looks similar to me, although I also agree that we should focus on the sub selects first and then introduce a separate |
You may want to look into |
That's right, this looks similar. But both queries can already be parsed without any feature flag.
Thanks for the hint, I will have a look at this one. |
Do you know the feeling, when you discover that apparently you have done something exceptionally in the past -- but you can't remember, when and why? :-D |
I've had a look at |
Greetings! What will we d with this PR please? |
I've been waiting all the time for you reply on my previous comment on this. Since this is quite a little bit of work and I don't want to do any wasted effort, I wanted to discuss if my intention matches your opinion. |
Sorry, I have not been fully aware of this.
Yes, lets just get a version compiling please. |
Yes, I am currently working on it, but due to a change in the master branch (probably this one dc14c4b) where no |
As already mentioned in one of my previous issues, I'd like to implement support for the Exasol syntax. For this, I will provide multiple PRs over time.
This is the first one, implementing
REGEXP_LIKE
support and allowing subselects without parentheses being part of functions with multiple parameters.Hint: Hide whitespace changes to see the actual changes not related to formatting