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

refactor: validate constraints eagerly #3472

Merged
merged 6 commits into from Mar 12, 2024

Conversation

tisonkun
Copy link
Contributor

I hereby agree to the terms of the GreptimeDB CLA.

Refer to a related PR or issue link (optional)

What's changed and what's your intention?

TRIVIAL AS IS.

Previously, we can check time index constraint for CREATE EXTERNAL TABLE on parsing (but we do on execution).

Now we check constraints once the condition value are finalized.

Checklist

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.
  • This PR does not require documentation updates.

Signed-off-by: tison <wander4096@gmail.com>
@github-actions github-actions bot added the docs-not-required This change does not impact docs. label Mar 10, 2024
@tisonkun tisonkun changed the title chore: validate constraints eagerly refacor: validate constraints eagerly Mar 10, 2024
@tisonkun tisonkun changed the title refacor: validate constraints eagerly refactor: validate constraints eagerly Mar 10, 2024
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Copy link

codecov bot commented Mar 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.93%. Comparing base (58bd065) to head (c243c6c).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3472      +/-   ##
==========================================
- Coverage   85.35%   84.93%   -0.43%     
==========================================
  Files         903      903              
  Lines      149639   149637       -2     
==========================================
- Hits       127726   127092     -634     
- Misses      21913    22545     +632     

Signed-off-by: tison <wander4096@gmail.com>
@MichaelScofield
Copy link
Collaborator

Can you add a case for #3471 in sqlness test?

@tisonkun
Copy link
Contributor Author

@MichaelScofield This doesn't fix #3471. Just a front code refactor.

Do you suggest we anyway add a case to indicate the current LTA manner described in #3471?

@MichaelScofield
Copy link
Collaborator

What's the meaning of "LTA"?

I mean to add a case in sqlness test that if inputting

CREATE TABLE IF NOT EXIST t(`Hits` STRING NULL, ts TIMESTAMP(3) NOT NULL, TIME INDEX ("ts"));

, the db returns msg indicating it's the "IF NOT EXIST" syntax error instead of "ERROR 1815 (HY000): Missing time index constraint".

@tisonkun
Copy link
Contributor Author

What's the meaning of "LTA"?

Less than awesome; but not a bug.

the db returns msg indicating it's the "IF NOT EXIST" syntax error instead of "ERROR 1815 (HY000): Missing time index constraint".

No. This patch doesn't fix #3471. It's a code refactor that possibly makes fixing #3471 easier. See #3471 (comment) for an explain.

@MichaelScofield
Copy link
Collaborator

After this PR, what's the error msg if user inputs "create table foo if not exist ..."?

@tisonkun
Copy link
Contributor Author

After this PR, what's the error msg if user inputs "create table foo if not exist ..."?

public=> create table if not exists t(ts timestamp time index);
OK 0
public=> create table if not exist t(ts timestamp time index);
ERROR:  Missing time index constraint
public=> 

@tisonkun tisonkun requested a review from waynexia March 12, 2024 04:31
@waynexia
Copy link
Member

After this PR, what's the error msg if user inputs "create table foo if not exist ..."?

public=> create table if not exists t(ts timestamp time index);
OK 0
public=> create table if not exist t(ts timestamp time index);
ERROR:  Missing time index constraint
public=> 

This error message looks more confusing... Can we avoid it?

@tisonkun
Copy link
Contributor Author

This error message looks more confusing... Can we avoid it?

This is what #3471 tracked and what I'd try to resolve later. Please read the background. I have explained this purpose many times here 🤣

Signed-off-by: tison <wander4096@gmail.com>
@waynexia waynexia enabled auto-merge March 12, 2024 12:38
@waynexia waynexia added this pull request to the merge queue Mar 12, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 12, 2024
@tisonkun tisonkun added this pull request to the merge queue Mar 12, 2024
Merged via the queue into GreptimeTeam:main with commit cafb470 Mar 12, 2024
18 checks passed
@tisonkun tisonkun deleted the improve-validate branch March 12, 2024 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-not-required This change does not impact docs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants