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(griffin): timestamp comparison operators #796

Merged
merged 2 commits into from
Feb 5, 2021

Conversation

piotrrzysko
Copy link
Contributor

Resolves #582.

I’ve made two decisions that might not be obvious, so please let me know if they are correct:

  1. Parsing variable string errors are silently ignored. For example:
create table sample_table( t1 timestamp, t2 timestamp, s1 string) timestamp(t2);
insert into sample_table values(systimestamp(), systimestamp(), '2020');
insert into sample_table values(systimestamp(), systimestamp(), 'abc');

Even though abc is not a valid timestamp the following query wouldn't fail, instead it will return only matched results:

select * from sample_table where t1 = s1;
  1. Currently operators = and != handle Exact timestamp and Time range. Time range with modifier and Explicit range are not supported - I am not sure if they are in the scope of the issue.

Additional changes:

  • In my opinion, the previous implementation of creating negated operators had a bug. Because of the fact that function factories are shared between threads using the flag isNegated from AbstractBooleanFunctionFactory may lead to race conditions. This PR fixes it.

@CLAassistant
Copy link

CLAassistant commented Feb 3, 2021

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@bluestreak01 bluestreak01 left a comment

Choose a reason for hiding this comment

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

Two small comments, to reduce object churn.

  • you are correct about the bug with isNegated, well spotted!
  • timestamp = and != should have the same semantics in function as they do when optimizer bypasses function calls. Implementing this would oversaturate this PR I feel.

Overall the PR is excellent and adds tremendous value! Thanks!

FunctionFactory greaterThan = createSwappingFactory(">", factory);
// `a < b` == `b > a`
addFactoryToList(factories, greaterThan);
// `b > a` == `b <= a`
Copy link
Member

Choose a reason for hiding this comment

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

b > a == !(b <= a)

@@ -196,6 +196,27 @@ public static int validateSignatureAndGetNameSeparator(String sig) throws SqlExc
return openBraceIndex;
}

public static String createSignatureWithSwappedArgs(String name, String signature) throws SqlException {
Copy link
Member

Choose a reason for hiding this comment

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

we can perhaps have here a reusable StringBuilder, one for all functions processed by the FunctionFactoryCache


public static String createSignature(String name, String signature) throws SqlException {
int openBraceIndex = validateSignatureAndGetNameSeparator(signature);
return name + signature.substring(openBraceIndex);
Copy link
Member

Choose a reason for hiding this comment

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

here too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

date comparison operators
3 participants