Re: Pull requests to review
I don't mean to fire a Volcano rule. I mean to just reuse the code in a creative way for the purpose of SqlUserDefinedTableMacro.coerce Currently, SqlUserDefinedTableMacro.coerce knows certain hard-coded cases, however it would probably be better if the conversion could be just reused from already existing in Calcite code. Vladimir
Re: Pull requests to review
Vladimir, could you elaborate more on this? Should such mentioned rule convert the RexLiterals with TimestampString value to Long with the time zone from data context? Are you sure that such rule wouldbe triggered before the logic SqlUserDefinedTableMacro.coerce(SqlUserDefinedTableMacro.java:190)? And finally - I thought that RelOptRule is an entry for optimization and not narrowing to proper execution. What if planner would not choose to apply such rule - we would have a case in which some expressions with functions tables could be computed properly while other wont. Maybe better to take this discussion to CALCITE-2641? On Tue, Nov 6, 2018 at 1:46 PM Vladimir Sitnikov < sitnikov.vladi...@gmail.com> wrote: > >There is no explicit time zone here provided, only the implicit one based > on > > There's connection-default time zone > (see org.apache.calcite.DataContext.Variable#TIME_ZONE ) > > I think the way to go there is: > 1) Convert SqlNod to Rex via SqlToRelConverter > 2) Evaluate the Rex with something like ReduceExpressionsRule with > exception that non-constant parameters are replaced with nulls > (ReduceExpressionsRule has a machinery to determine if expression is a > constant or not) > > That would make the thing consistent with existing computations/casts > (which are tricky to get right). > > Vladimir > -- Piotr Bojko http://about.me/ptr.bojko
Re: Pull requests to review
>There is no explicit time zone here provided, only the implicit one based on There's connection-default time zone (see org.apache.calcite.DataContext.Variable#TIME_ZONE ) I think the way to go there is: 1) Convert SqlNod to Rex via SqlToRelConverter 2) Evaluate the Rex with something like ReduceExpressionsRule with exception that non-constant parameters are replaced with nulls (ReduceExpressionsRule has a machinery to determine if expression is a constant or not) That would make the thing consistent with existing computations/casts (which are tricky to get right). Vladimir
Re: Pull requests to review
Correct me if I am wrong. For test UDFTest.testDate line ~823 conversion from TimestampString (TIMESTAMP '1970-01-01 00:00:00') is done via a call to RexLiteral.getValueAs(Long.class). There is no explicit time zone here provided, only the implicit one based on TimestampString.getMillisSinceEpoch method definition. Should the table functions implementation for timestamp parameters support the same thinking as for other UDFs? On Tue, Nov 6, 2018 at 12:44 AM Julian Hyde wrote: > A TimestampString has no timezone. It does not represent a moment in time. > It represents a position of the hands on a clock. > > A Timestamp does not have a timezone either, but it represents a moment in > time. (Internally represented by milliseconds since the UTC epoch.) > > Therefore, to go from a TimestampString to a Timestamp, or vice versa, you > need a time zone. I’m not sure where that should come from in your code. > > Julian > > > > On Nov 5, 2018, at 1:19 PM, ptr.bo...@gmail.com wrote: > > > > Julian, so.. is it correct to translate TimestampString to > > java.sql.Timestamp on RexToLixTranslator.convert(...) level as in pull > > request https://github.com/apache/calcite/pull/900/commits ? > > > > > > I hope so, then I could revoke pull request > > https://github.com/apache/calcite/pull/878 > > > > > > On Mon, Nov 5, 2018 at 9:34 PM Julian Hyde wrote: > > > >> Yeah, TimestampString is for SqlNode and RexNode only. Not JDBC code. > >> Likewise DateString, TimeString, NlsString. Sorry the doc didn’t make > that > >> clear. Although frankly it’s impractical to document all of the places > >> something is NOT used. > >> > >>> On Nov 5, 2018, at 5:49 AM, ptr.bo...@gmail.com wrote: > >>> > >>> Vladimir, > >>> > >>> I thought that TimestampString IS the appropriate type :D (knowledge > >> based > >>> on Rexbuilder code, where TimestampString is being created as > >> RexLiteral). > >>> > >>> My problem is that I don't see what is the scope of TimestampString, > >>> DateString, etc in Calcite. Does it span to Rel/Rex tree? Or it should > >> not? > >>> This is why I've created two different patches :( > >>> > >>> Any help with the responsibility of TimestampString appreciated. > Without > >> it > >>> - bug is still there and I could create lots of other mishit patches. > >> > >> > > > > -- > > Piotr Bojko > > http://about.me/ptr.bojko > > -- Piotr Bojko http://about.me/ptr.bojko
Re: Pull requests to review
A TimestampString has no timezone. It does not represent a moment in time. It represents a position of the hands on a clock. A Timestamp does not have a timezone either, but it represents a moment in time. (Internally represented by milliseconds since the UTC epoch.) Therefore, to go from a TimestampString to a Timestamp, or vice versa, you need a time zone. I’m not sure where that should come from in your code. Julian > On Nov 5, 2018, at 1:19 PM, ptr.bo...@gmail.com wrote: > > Julian, so.. is it correct to translate TimestampString to > java.sql.Timestamp on RexToLixTranslator.convert(...) level as in pull > request https://github.com/apache/calcite/pull/900/commits ? > > > I hope so, then I could revoke pull request > https://github.com/apache/calcite/pull/878 > > > On Mon, Nov 5, 2018 at 9:34 PM Julian Hyde wrote: > >> Yeah, TimestampString is for SqlNode and RexNode only. Not JDBC code. >> Likewise DateString, TimeString, NlsString. Sorry the doc didn’t make that >> clear. Although frankly it’s impractical to document all of the places >> something is NOT used. >> >>> On Nov 5, 2018, at 5:49 AM, ptr.bo...@gmail.com wrote: >>> >>> Vladimir, >>> >>> I thought that TimestampString IS the appropriate type :D (knowledge >> based >>> on Rexbuilder code, where TimestampString is being created as >> RexLiteral). >>> >>> My problem is that I don't see what is the scope of TimestampString, >>> DateString, etc in Calcite. Does it span to Rel/Rex tree? Or it should >> not? >>> This is why I've created two different patches :( >>> >>> Any help with the responsibility of TimestampString appreciated. Without >> it >>> - bug is still there and I could create lots of other mishit patches. >> >> > > -- > Piotr Bojko > http://about.me/ptr.bojko
Re: Pull requests to review
Julian, so.. is it correct to translate TimestampString to java.sql.Timestamp on RexToLixTranslator.convert(...) level as in pull request https://github.com/apache/calcite/pull/900/commits ? I hope so, then I could revoke pull request https://github.com/apache/calcite/pull/878 On Mon, Nov 5, 2018 at 9:34 PM Julian Hyde wrote: > Yeah, TimestampString is for SqlNode and RexNode only. Not JDBC code. > Likewise DateString, TimeString, NlsString. Sorry the doc didn’t make that > clear. Although frankly it’s impractical to document all of the places > something is NOT used. > > > On Nov 5, 2018, at 5:49 AM, ptr.bo...@gmail.com wrote: > > > > Vladimir, > > > > I thought that TimestampString IS the appropriate type :D (knowledge > based > > on Rexbuilder code, where TimestampString is being created as > RexLiteral). > > > > My problem is that I don't see what is the scope of TimestampString, > > DateString, etc in Calcite. Does it span to Rel/Rex tree? Or it should > not? > > This is why I've created two different patches :( > > > > Any help with the responsibility of TimestampString appreciated. Without > it > > - bug is still there and I could create lots of other mishit patches. > > -- Piotr Bojko http://about.me/ptr.bojko
Re: Pull requests to review
Yeah, TimestampString is for SqlNode and RexNode only. Not JDBC code. Likewise DateString, TimeString, NlsString. Sorry the doc didn’t make that clear. Although frankly it’s impractical to document all of the places something is NOT used. > On Nov 5, 2018, at 5:49 AM, ptr.bo...@gmail.com wrote: > > Vladimir, > > I thought that TimestampString IS the appropriate type :D (knowledge based > on Rexbuilder code, where TimestampString is being created as RexLiteral). > > My problem is that I don't see what is the scope of TimestampString, > DateString, etc in Calcite. Does it span to Rel/Rex tree? Or it should not? > This is why I've created two different patches :( > > Any help with the responsibility of TimestampString appreciated. Without it > - bug is still there and I could create lots of other mishit patches.
Re: Pull requests to review
Vladimir, I thought that TimestampString IS the appropriate type :D (knowledge based on Rexbuilder code, where TimestampString is being created as RexLiteral). My problem is that I don't see what is the scope of TimestampString, DateString, etc in Calcite. Does it span to Rel/Rex tree? Or it should not? This is why I've created two different patches :( Any help with the responsibility of TimestampString appreciated. Without it - bug is still there and I could create lots of other mishit patches.
Re: Pull requests to review
Piotr, Have you tried to use SqlToRelConverter to convert argument from SqlNode format to the appropriate one? Vladimir
