[jira] [Commented] (CALCITE-2339) JDBC adapter should transform timestamp arithmetic for target database
[ https://issues.apache.org/jira/browse/CALCITE-2339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16764016#comment-16764016 ] James Duong commented on CALCITE-2339: -- Thanks! I've rebased on top of the fix for CALCITE-2836. > JDBC adapter should transform timestamp arithmetic for target database > --- > > Key: CALCITE-2339 > URL: https://issues.apache.org/jira/browse/CALCITE-2339 > Project: Calcite > Issue Type: Bug > Components: jdbc-adapter >Reporter: Julian Hyde >Assignee: Julian Hyde >Priority: Major > Labels: pull-request-available > Attachments: Datetime Addition - Calcite.pdf > > Time Spent: 10m > Remaining Estimate: 0h > > JDBC adapter should transform timestamp arithmetic for target database. > There are two ways in Calcite to add intervals to timestamps: the > TIMESTAMP_ADD function and the " + " operator. > The attached document (authored by James Doung) describes their pros and cons. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-2339) JDBC adapter should transform timestamp arithmetic for target database
[ https://issues.apache.org/jira/browse/CALCITE-2339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16763967#comment-16763967 ] Julian Hyde commented on CALCITE-2339: -- 2836 was the cause of the breakage in the master branch; it has now been fixed, so you can rebase the PR for 2339. > JDBC adapter should transform timestamp arithmetic for target database > --- > > Key: CALCITE-2339 > URL: https://issues.apache.org/jira/browse/CALCITE-2339 > Project: Calcite > Issue Type: Bug > Components: jdbc-adapter >Reporter: Julian Hyde >Assignee: Julian Hyde >Priority: Major > Labels: pull-request-available > Attachments: Datetime Addition - Calcite.pdf > > Time Spent: 10m > Remaining Estimate: 0h > > JDBC adapter should transform timestamp arithmetic for target database. > There are two ways in Calcite to add intervals to timestamps: the > TIMESTAMP_ADD function and the " + " operator. > The attached document (authored by James Doung) describes their pros and cons. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-2339) JDBC adapter should transform timestamp arithmetic for target database
[ https://issues.apache.org/jira/browse/CALCITE-2339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16763889#comment-16763889 ] Kevin Risden commented on CALCITE-2339: --- Travis CI should be fixed by CALCITE-2836. It just went into master. > JDBC adapter should transform timestamp arithmetic for target database > --- > > Key: CALCITE-2339 > URL: https://issues.apache.org/jira/browse/CALCITE-2339 > Project: Calcite > Issue Type: Bug > Components: jdbc-adapter >Reporter: Julian Hyde >Assignee: Julian Hyde >Priority: Major > Labels: pull-request-available > Attachments: Datetime Addition - Calcite.pdf > > Time Spent: 10m > Remaining Estimate: 0h > > JDBC adapter should transform timestamp arithmetic for target database. > There are two ways in Calcite to add intervals to timestamps: the > TIMESTAMP_ADD function and the " + " operator. > The attached document (authored by James Doung) describes their pros and cons. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-2339) JDBC adapter should transform timestamp arithmetic for target database
[ https://issues.apache.org/jira/browse/CALCITE-2339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16763767#comment-16763767 ] James Duong commented on CALCITE-2339: -- The build for this PR now fails after merging from master in TravisCI. It fails on the Calcite Plus module, which wasn't changed as part of this work [https://travis-ci.org/apache/calcite/builds/490219956?utm_source=github_status_medium=notification] Master itself is failing Travis CI currently: https://travis-ci.org/apache/calcite/jobs/489675325 > JDBC adapter should transform timestamp arithmetic for target database > --- > > Key: CALCITE-2339 > URL: https://issues.apache.org/jira/browse/CALCITE-2339 > Project: Calcite > Issue Type: Bug > Components: jdbc-adapter >Reporter: Julian Hyde >Assignee: Julian Hyde >Priority: Major > Labels: pull-request-available > Attachments: Datetime Addition - Calcite.pdf > > Time Spent: 10m > Remaining Estimate: 0h > > JDBC adapter should transform timestamp arithmetic for target database. > There are two ways in Calcite to add intervals to timestamps: the > TIMESTAMP_ADD function and the " + " operator. > The attached document (authored by James Doung) describes their pros and cons. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-2339) JDBC adapter should transform timestamp arithmetic for target database
[ https://issues.apache.org/jira/browse/CALCITE-2339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16761955#comment-16761955 ] James Duong commented on CALCITE-2339: -- Thanks Julian. I'll work on updating the PR for this to master. > JDBC adapter should transform timestamp arithmetic for target database > --- > > Key: CALCITE-2339 > URL: https://issues.apache.org/jira/browse/CALCITE-2339 > Project: Calcite > Issue Type: Bug > Components: jdbc-adapter >Reporter: Julian Hyde >Assignee: Julian Hyde >Priority: Major > Labels: pull-request-available > Attachments: Datetime Addition - Calcite.pdf > > > JDBC adapter should transform timestamp arithmetic for target database. > There are two ways in Calcite to add intervals to timestamps: the > TIMESTAMP_ADD function and the " + " operator. > The attached document (authored by James Doung) describes their pros and cons. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-2339) JDBC adapter should transform timestamp arithmetic for target database
[ https://issues.apache.org/jira/browse/CALCITE-2339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16727591#comment-16727591 ] Julian Hyde commented on CALCITE-2339: -- We failed to review and get this into 1.18. I'm sorry. I've added the label 'pull-request-available' and we'll try to do better. > JDBC adapter should transform timestamp arithmetic for target database > --- > > Key: CALCITE-2339 > URL: https://issues.apache.org/jira/browse/CALCITE-2339 > Project: Calcite > Issue Type: Bug > Components: jdbc-adapter >Reporter: Julian Hyde >Assignee: Julian Hyde >Priority: Major > Labels: pull-request-available > Attachments: Datetime Addition - Calcite.pdf > > > JDBC adapter should transform timestamp arithmetic for target database. > There are two ways in Calcite to add intervals to timestamps: the > TIMESTAMP_ADD function and the " + " operator. > The attached document (authored by James Doung) describes their pros and cons. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-2339) JDBC adapter should transform timestamp arithmetic for target database
[ https://issues.apache.org/jira/browse/CALCITE-2339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16522722#comment-16522722 ] Julian Hyde commented on CALCITE-2339: -- Will try to review and merge before 1.17. > JDBC adapter should transform timestamp arithmetic for target database > --- > > Key: CALCITE-2339 > URL: https://issues.apache.org/jira/browse/CALCITE-2339 > Project: Calcite > Issue Type: Bug > Components: jdbc-adapter >Reporter: Julian Hyde >Assignee: Julian Hyde >Priority: Major > Fix For: 1.17.0 > > Attachments: Datetime Addition - Calcite.pdf > > > JDBC adapter should transform timestamp arithmetic for target database. > There are two ways in Calcite to add intervals to timestamps: the > TIMESTAMP_ADD function and the " + " operator. > The attached document (authored by James Doung) describes their pros and cons. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-2339) JDBC adapter should transform timestamp arithmetic for target database
[ https://issues.apache.org/jira/browse/CALCITE-2339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16513602#comment-16513602 ] James Duong commented on CALCITE-2339: -- PR has been updated. I looked into using CAST, but did not see a way I could provide a RexExecutor to this utility from RelToSqlConverter to apply const reduction. > JDBC adapter should transform timestamp arithmetic for target database > --- > > Key: CALCITE-2339 > URL: https://issues.apache.org/jira/browse/CALCITE-2339 > Project: Calcite > Issue Type: Bug >Reporter: Julian Hyde >Assignee: Julian Hyde >Priority: Major > Attachments: Datetime Addition - Calcite.pdf > > > JDBC adapter should transform timestamp arithmetic for target database. > There are two ways in Calcite to add intervals to timestamps: the > TIMESTAMP_ADD function and the " + " operator. > The attached document (authored by James Doung) describes their pros and cons. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-2339) JDBC adapter should transform timestamp arithmetic for target database
[ https://issues.apache.org/jira/browse/CALCITE-2339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16511707#comment-16511707 ] James Duong commented on CALCITE-2339: -- * missing MINUTE from the switch * >>> done * I don't think the visitor can throw FoundOne * >>> the DateTimePlusToTsAddVisitor can throw a FoundOne() by design - if it runs into a multi-field interval since we can't transform that to timestampAdd ([https://github.com/jduo/calcite/blob/16e9e46c0db8e46a5e14beed208ca486f24bf09a/core/src/main/java/org/apache/calcite/rex/RexUtil.java#L2711|https://github.com/jduo/calcite/blob/16e9e46c0db8e46a5e14beed208ca486f24bf09a/core/src/main/java/org/apache/calcite/rex/RexUtil.java#L2711)] ) * only call the method if the condition is not null, do not check for nulls in the method, and assume the method returns not-null * >>> assuming this is in reference to the convertDatetimePlusToTimestampAdd() method. I would like to have the semantics to return either the transformation is not possible, or the completed transformation. Would a Java8 optional be more appropriate? * indentation is off in a couple of places * >>> I'll check this out. * need some unit tests * >>> Will do. * rather than trying to do constant reduction to convert interval to integer, could you just use "cast( AS INTEGER)"? less code to maintain * >>> I thought about this, but realized that if we ever need to invoke this transformation to timestampadd, it means the backend probably doesn't support intervals so you can't use CAST(). What might work is to create the Cast node, then use RexSimplify to see if it can be constant-reduced to just an integer literal. I'll look into this. > JDBC adapter should transform timestamp arithmetic for target database > --- > > Key: CALCITE-2339 > URL: https://issues.apache.org/jira/browse/CALCITE-2339 > Project: Calcite > Issue Type: Bug >Reporter: Julian Hyde >Assignee: Julian Hyde >Priority: Major > Attachments: Datetime Addition - Calcite.pdf > > > JDBC adapter should transform timestamp arithmetic for target database. > There are two ways in Calcite to add intervals to timestamps: the > TIMESTAMP_ADD function and the " + " operator. > The attached document (authored by James Doung) describes their pros and cons. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-2339) JDBC adapter should transform timestamp arithmetic for target database
[ https://issues.apache.org/jira/browse/CALCITE-2339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16502245#comment-16502245 ] Julian Hyde commented on CALCITE-2339: -- Looks basically good: * missing MINUTE from the switch * I don't think the visitor can throw FoundOne * only call the method if the condition is not null, do not check for nulls in the method, and assume the method returns not-null * indentation is off in a couple of places * need some unit tests * rather than trying to do constant reduction to convert interval to integer, could you just use "cast( AS INTEGER)"? less code to maintain > JDBC adapter should transform timestamp arithmetic for target database > --- > > Key: CALCITE-2339 > URL: https://issues.apache.org/jira/browse/CALCITE-2339 > Project: Calcite > Issue Type: Bug >Reporter: Julian Hyde >Assignee: Julian Hyde >Priority: Major > Attachments: Datetime Addition - Calcite.pdf > > > JDBC adapter should transform timestamp arithmetic for target database. > There are two ways in Calcite to add intervals to timestamps: the > TIMESTAMP_ADD function and the " + " operator. > The attached document (authored by James Doung) describes their pros and cons. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-2339) JDBC adapter should transform timestamp arithmetic for target database
[ https://issues.apache.org/jira/browse/CALCITE-2339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16501595#comment-16501595 ] James Duong commented on CALCITE-2339: -- I've put up a PR for what I'm thinking should happen: [https://github.com/apache/calcite/pull/721] (This is an initial implementation to demonstrate the idea. I intend to add more tests for this). This PR adds a new RexUtil method to convert a RexNode holding a datetime_plus call to an equivalent RexNode holding a TimestampAdd call. I've changed RelToSqlConverter to check if a SqlDialect needs this transformation and apply it. However this introduces an odd bit of behavior where we need to use RexBuilder from within RelToSqlConverter (because the conversion process creates RexNodes, but we can't check if a dialect needs this conversion until we're in RelToSqlConverter already). This patch handles interval literals as well as multiplications of interval literals. (the latter needed because the TimestampAddConvertlet creates a multiplication expression when normalizing a TimestampAdd call to a datetime_plus call). I also noticed that CALCITE-2188 already implements dialect-level unparsing of datetime_plus to timestampadd. However this is only applicable to SqlDialects. Also, it doesn't appear to handle multiplications of interval literals. > JDBC adapter should transform timestamp arithmetic for target database > --- > > Key: CALCITE-2339 > URL: https://issues.apache.org/jira/browse/CALCITE-2339 > Project: Calcite > Issue Type: Bug >Reporter: Julian Hyde >Assignee: Julian Hyde >Priority: Major > Attachments: Datetime Addition - Calcite.pdf > > > JDBC adapter should transform timestamp arithmetic for target database. > There are two ways in Calcite to add intervals to timestamps: the > TIMESTAMP_ADD function and the " + " operator. > The attached document (authored by James Doung) describes their pros and cons. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-2339) JDBC adapter should transform timestamp arithmetic for target database
[ https://issues.apache.org/jira/browse/CALCITE-2339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16495921#comment-16495921 ] Laurent Goujon commented on CALCITE-2339: - {quote} Laurent Goujon, That would be quite a significant change, because DATETIME_PLUS only takes two arguments: a timestamp and an interval. And WEEK is not a supported unit of interval types. Are you proposing to add an extra parameter? {quote} Not an extra argument, but it looks like {{IntervalSqlType}} operand is derived from {{SqlIntervalQualifier}}, and that {{WEEK}} is could a possible interval qualifier (it looks like that {{SqlIntervalQualifier#typeName()}} returns more than just SQL intervals). So even if {{WEEK}} is not a valid interval type from a parsing/validation point of view, any drawback of creating a call with a {{WEEK}} {{IntervalSqlType}} operand in the convertlet? > JDBC adapter should transform timestamp arithmetic for target database > --- > > Key: CALCITE-2339 > URL: https://issues.apache.org/jira/browse/CALCITE-2339 > Project: Calcite > Issue Type: Bug >Reporter: Julian Hyde >Assignee: Julian Hyde >Priority: Major > Attachments: Datetime Addition - Calcite.pdf > > > JDBC adapter should transform timestamp arithmetic for target database. > There are two ways in Calcite to add intervals to timestamps: the > TIMESTAMP_ADD function and the " + " operator. > The attached document (authored by James Doung) describes their pros and cons. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-2339) JDBC adapter should transform timestamp arithmetic for target database
[ https://issues.apache.org/jira/browse/CALCITE-2339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16495902#comment-16495902 ] James Duong commented on CALCITE-2339: -- Tableau does make use of non-interval units such as WEEK and QUARTER. > JDBC adapter should transform timestamp arithmetic for target database > --- > > Key: CALCITE-2339 > URL: https://issues.apache.org/jira/browse/CALCITE-2339 > Project: Calcite > Issue Type: Bug >Reporter: Julian Hyde >Assignee: Julian Hyde >Priority: Major > Attachments: Datetime Addition - Calcite.pdf > > > JDBC adapter should transform timestamp arithmetic for target database. > There are two ways in Calcite to add intervals to timestamps: the > TIMESTAMP_ADD function and the " + " operator. > The attached document (authored by James Doung) describes their pros and cons. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-2339) JDBC adapter should transform timestamp arithmetic for target database
[ https://issues.apache.org/jira/browse/CALCITE-2339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16495888#comment-16495888 ] Julian Hyde commented on CALCITE-2339: -- My point about WEEK is that 99.2% (I'm guessing) of intervals in real-world SQL are one of the core interval types. So WEEK is not really worth worrying about. > JDBC adapter should transform timestamp arithmetic for target database > --- > > Key: CALCITE-2339 > URL: https://issues.apache.org/jira/browse/CALCITE-2339 > Project: Calcite > Issue Type: Bug >Reporter: Julian Hyde >Assignee: Julian Hyde >Priority: Major > Attachments: Datetime Addition - Calcite.pdf > > > JDBC adapter should transform timestamp arithmetic for target database. > There are two ways in Calcite to add intervals to timestamps: the > TIMESTAMP_ADD function and the " + " operator. > The attached document (authored by James Doung) describes their pros and cons. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-2339) JDBC adapter should transform timestamp arithmetic for target database
[ https://issues.apache.org/jira/browse/CALCITE-2339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16495885#comment-16495885 ] Julian Hyde commented on CALCITE-2339: -- [~laurentgo], That would be quite a significant change, because DATETIME_PLUS only takes two arguments: a timestamp and an interval. And WEEK is not a supported unit of interval types. Are you proposing to add an extra parameter? [~jduong], You could simplify {{integer_column * 1}} to {{integer_column}}. > JDBC adapter should transform timestamp arithmetic for target database > --- > > Key: CALCITE-2339 > URL: https://issues.apache.org/jira/browse/CALCITE-2339 > Project: Calcite > Issue Type: Bug >Reporter: Julian Hyde >Assignee: Julian Hyde >Priority: Major > Attachments: Datetime Addition - Calcite.pdf > > > JDBC adapter should transform timestamp arithmetic for target database. > There are two ways in Calcite to add intervals to timestamps: the > TIMESTAMP_ADD function and the " + " operator. > The attached document (authored by James Doung) describes their pros and cons. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-2339) JDBC adapter should transform timestamp arithmetic for target database
[ https://issues.apache.org/jira/browse/CALCITE-2339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16495873#comment-16495873 ] Laurent Goujon commented on CALCITE-2339: - Could we change the {{DATETIME_PLUS}} operator to support non-standard/internal interval types so that WEEK can be represented as such and being preserved up to each adapter? > JDBC adapter should transform timestamp arithmetic for target database > --- > > Key: CALCITE-2339 > URL: https://issues.apache.org/jira/browse/CALCITE-2339 > Project: Calcite > Issue Type: Bug >Reporter: Julian Hyde >Assignee: Julian Hyde >Priority: Major > Attachments: Datetime Addition - Calcite.pdf > > > JDBC adapter should transform timestamp arithmetic for target database. > There are two ways in Calcite to add intervals to timestamps: the > TIMESTAMP_ADD function and the " + " operator. > The attached document (authored by James Doung) describes their pros and cons. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-2339) JDBC adapter should transform timestamp arithmetic for target database
[ https://issues.apache.org/jira/browse/CALCITE-2339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16495869#comment-16495869 ] James Duong commented on CALCITE-2339: -- The other downside I see: If someone used TIMESTAMPADD(ts, integer_column, DAY), we'd have to transform this to ts + integer_column * Interval '1' DAY, then transform it back to TIMESTAMPADD(ts, integer_column * 1, DAY) or if the interval's a non-literal: TIMESTAMPADD(ts, integer_column * cast(interval_expression as integer), DAY) > JDBC adapter should transform timestamp arithmetic for target database > --- > > Key: CALCITE-2339 > URL: https://issues.apache.org/jira/browse/CALCITE-2339 > Project: Calcite > Issue Type: Bug >Reporter: Julian Hyde >Assignee: Julian Hyde >Priority: Major > Attachments: Datetime Addition - Calcite.pdf > > > JDBC adapter should transform timestamp arithmetic for target database. > There are two ways in Calcite to add intervals to timestamps: the > TIMESTAMP_ADD function and the " + " operator. > The attached document (authored by James Doung) describes their pros and cons. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-2339) JDBC adapter should transform timestamp arithmetic for target database
[ https://issues.apache.org/jira/browse/CALCITE-2339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16495832#comment-16495832 ] Julian Hyde commented on CALCITE-2339: -- I propose that we continue to use interval "+" as the internal representation, but writer a converter from an interval "+" expression to a TIMESTAMPADD expression that can be used by various dialects in the JDBC adapter. The only downside I can see is that someone might write {{TIMESTAMPADD(ts, 3, WEEK)}} and see {{TIMESTAMPADD(ts, 21, DAY)}} sent to the underlying DB. That's not too bad. The standard intervals day, month, year, hour, second etc will not be affected. > JDBC adapter should transform timestamp arithmetic for target database > --- > > Key: CALCITE-2339 > URL: https://issues.apache.org/jira/browse/CALCITE-2339 > Project: Calcite > Issue Type: Bug >Reporter: Julian Hyde >Assignee: Julian Hyde >Priority: Major > Attachments: Datetime Addition - Calcite.pdf > > > JDBC adapter should transform timestamp arithmetic for target database. > There are two ways in Calcite to add intervals to timestamps: the > TIMESTAMP_ADD function and the " + " operator. > The attached document (authored by James Doung) describes their pros and cons. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-2339) JDBC adapter should transform timestamp arithmetic for target database
[ https://issues.apache.org/jira/browse/CALCITE-2339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16495824#comment-16495824 ] James Duong commented on CALCITE-2339: -- Note that it's possible to represent multi-field intervals with timestampadd. You just nest multiple timestampadd calls together for each unit. Need to apply the EXTRACT function to get individual units out of an arbitrary interval expression though. > JDBC adapter should transform timestamp arithmetic for target database > --- > > Key: CALCITE-2339 > URL: https://issues.apache.org/jira/browse/CALCITE-2339 > Project: Calcite > Issue Type: Bug >Reporter: Julian Hyde >Assignee: Julian Hyde >Priority: Major > Attachments: Datetime Addition - Calcite.pdf > > > JDBC adapter should transform timestamp arithmetic for target database. > There are two ways in Calcite to add intervals to timestamps: the > TIMESTAMP_ADD function and the " + " operator. > The attached document (authored by James Doung) describes their pros and cons. -- This message was sent by Atlassian JIRA (v7.6.3#76005)