Skip to content
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(sql): 'is null' is an alias for '= null' and 'is not null' for '!= null' #1967

Merged
merged 23 commits into from Mar 26, 2022

Conversation

marregui
Copy link
Contributor

@marregui marregui commented Mar 19, 2022

MindsDB relies on IS NULL and IS NOT NULL on column literals, e.g.:

create table s( i int);
insert into s values(0), (1), (null);
s where i = null;  -->  s where i IS NULL;
s where i != null; --> s where i IS NOT NULL;

as it rewrites queries to check for null values this way.

mysql> SELECT
    -> m.ts,
    -> d.number_of_rooms, d.location, d.neighborhood, d.days_on_market, m.rental_price,  m FROM questdb.house_rentals_data AS d JOIN mindsdb.home_rentals_model_ts as m
    -> WHERE m.ts > LATEST;
ERROR 1149 (HY000): Execution failed on sql 'SELECT *
FROM house_rentals_data AS d
WHERE d.ts IS NOT NULL ORDER BY d.ts DESC
 LIMIT 10': unexpected token: IS
LINE 3: WHERE d.ts IS NOT NULL ORDER BY d.ts DESC

The approach in this PR is to detect these tokens, IS [NOT] NULL while parsing expressions, wherever they are, and replace them for the equivalent operator token =, '!='.

@marregui marregui self-assigned this Mar 19, 2022
@marregui marregui changed the title feat(sql): Make 'is null' and 'is not null' by aliases for =,!= feat(sql): 'is null' is an alias for '= null' and 'is not null' for '!= null' Mar 20, 2022
@marregui marregui added the New feature Feature requests label Mar 20, 2022
@marregui marregui requested a review from puzpuzpuz March 20, 2022 16:42
Copy link
Contributor

@puzpuzpuz puzpuzpuz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW did we miss updating armosx/libquestdb.dylib in one of the previous PRs? I can see it's been updated here.

@bziobrowski
Copy link
Contributor

While it's ok to use = and != on null internally, in SQL it's practically useless and should always evaluate to NULL.
Examples :

1 = null -> NULL
null = 1 -> NULL
null = null NULL
1 != null NULL
null != null NULL

vs

1 is null -> FALSE
1 is not null -> TRUE
null is null -> TRUE
null is not null -> FALSE

@puzpuzpuz
Copy link
Contributor

puzpuzpuz commented Mar 21, 2022

While it's ok to use = and != on null internally, in SQL it's practically useless and should always evaluate to NULL.

The thing is that in our case this would be a breaking change since many of our users rely on = null/!= null instead of is null/is not null. In our SQL dialect, we don't support three-valued logic.

@marregui
Copy link
Contributor Author

BTW did we miss updating armosx/libquestdb.dylib in one of the previous PRs? I can see it's been updated here.

I had to build it locally because I was getting a linkage error (UnsatisfiedLink), related to https://questdb-tech.slack.com/archives/C018V7XJKH9/p1646763998810699

@marregui marregui requested a review from puzpuzpuz March 21, 2022 17:22
@marregui marregui requested a review from puzpuzpuz March 22, 2022 21:05
@marregui marregui requested a review from puzpuzpuz March 24, 2022 14:51
Copy link
Contributor

@puzpuzpuz puzpuzpuz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job! Left some comments, mostly nits

@marregui
Copy link
Contributor Author

thank you very much @puzpuzpuz and @bziobrowski for the thorough review and for all the help, I enjoyed the experience.

What are the rules for merging?, who presses the red button?

@bziobrowski
Copy link
Contributor

I don't have merge privilege so it has to be Andrey .

@puzpuzpuz
Copy link
Contributor

I don't have merge privilege so it has to be Andrey .

Neither do I. 😀

So, it has to be Miguel or someone else from the core team.

@ideoma
Copy link
Collaborator

ideoma commented Mar 26, 2022

[PR Coverage check]

😍 pass : 60 / 60 (100.00%)

file detail

path covered line new line coverage
🔵 io/questdb/cutlass/pgwire/PGConnectionContext.java 10 10 100.00%
🔵 io/questdb/griffin/SqlKeywords.java 5 5 100.00%
🔵 io/questdb/griffin/ExpressionParser.java 30 30 100.00%
🔵 io/questdb/griffin/engine/functions/eq/EqDoubleFunctionFactory.java 15 15 100.00%

@bluestreak01 bluestreak01 merged commit a9e3698 into master Mar 26, 2022
@bluestreak01 bluestreak01 deleted the ma/is-null branch March 26, 2022 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New feature Feature requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants