Re: Pull requests to review

[Vladimir Sitnikov]

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

[ptr.bo...@gmail.com]

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

[Vladimir Sitnikov]

>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

[ptr.bo...@gmail.com]

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

[Julian Hyde]

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

[ptr.bo...@gmail.com]

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 Hyde]

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

[ptr.bo...@gmail.com]

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 Sitnikov]

Piotr,

Have you tried to use SqlToRelConverter to convert argument from SqlNode
format to the appropriate one?

Vladimir

Re: Pull requests

[Julian Hyde]

Thanks for working on the Geode adapter PR. It looks as if our comments on the 
JIRA case have started things moving again.


> On Jan 30, 2018, at 12:45 PM, Michael Mior  wrote:
> 
> I've been working on the Geode adapter but I'm waiting for a response from
> Christian. I merged the updates to calcite-test-dataset but the integration
> tests in the PR are failing and there's a few stylistic issues that need to
> be resolved.
> 
> --
> Michael Mior
> mm...@apache.org
> 
> 2018-01-30 15:40 GMT-05:00 Julian Hyde :
> 
>> Can someone please review and commit https://issues.apache.org/
>> jira/browse/CALCITE-508 
>> (Avatica) and https://issues.apache.org/jira/browse/CALCITE-2059 <
>> https://issues.apache.org/jira/browse/CALCITE-2059> (Geode adapter). I am
>> burning out.
>> 
>> Also, we need an Avatica release.
>> 
>> Julian
>> 
>> 

Re: Pull requests

[Michael Mior]

I've been working on the Geode adapter but I'm waiting for a response from
Christian. I merged the updates to calcite-test-dataset but the integration
tests in the PR are failing and there's a few stylistic issues that need to
be resolved.

--
Michael Mior
mm...@apache.org

2018-01-30 15:40 GMT-05:00 Julian Hyde :

> Can someone please review and commit https://issues.apache.org/
> jira/browse/CALCITE-508 
> (Avatica) and https://issues.apache.org/jira/browse/CALCITE-2059 <
> https://issues.apache.org/jira/browse/CALCITE-2059> (Geode adapter). I am
> burning out.
>
> Also, we need an Avatica release.
>
> Julian
>
>

Re: Pull requests

[Jesus Camacho Rodriguez]

Michael,

Thanks a lot for taking care of this! I will take a look.

-Jesús 




On 6/18/17, 12:43 AM, "michael.m...@gmail.com on behalf of Michael Mior" 
 wrote:

>Pushed a fix and most of the Druid integration tests pass now. The one
>failure looks like it might be an issue with the test. Cassandra
>integration tests are also failing which seems to be a Calcite problem that
>I'm looking into. But I think the VM should be in a working state now if
>you pull the latest from calcite-test-dataset and rebuild it.
>
>--
>Michael Mior
>mm...@uwaterloo.ca
>
>2017-06-17 17:41 GMT-04:00 Michael Mior :
>
>> Taking a look at the VM now and I do see some Puppet issues. I'll try to
>> get that sorted out.
>>
>> --
>> Michael Mior
>> mm...@apache.org
>>
>> 2017-06-17 16:12 GMT-04:00 Julian Hyde :
>>
>>> Many thanks.
>>>
>>> Yes, there are several PRs waiting for the fix to the VM.
>>>
>>> On Fri, Jun 16, 2017 at 11:07 AM, Jesus Camacho Rodriguez
>>>  wrote:
>>> > Hi Julian,
>>> >
>>> > It slipped my mind to reply. Since I'll be acting as RM for 1.13, it
>>> makes sense that I monitor the PRs and respond to them.
>>> >
>>> > I will also try to fix the issue with the Druid VM before the release.
>>> >
>>> > Thanks,
>>> > Jesús
>>> >
>>> >
>>> >
>>> >
>>> >
>>> > On 6/16/17, 7:00 PM, "Julian Hyde"  wrote:
>>> >
>>> >>Anyone?
>>> >>
>>> >>On Thu, Jun 15, 2017 at 9:32 AM, Julian Hyde  wrote:
>>> >>> I’m going to be out for the next week or so. Can someone please
>>> volunteer to monitor the pull requests? I strive to give each pull request
>>> a response within 24 hours, and commit them when they’re ready.
>>> >>>
>>> >>> Slow responses to pull requests are very discouraging to potential
>>> contributors, so this is a critical task for our project.
>>> >>>
>>> >>> Julian
>>> >>>
>>> >>
>>>
>>
>>

Re: Pull requests

[Michael Mior]

Taking a look at the VM now and I do see some Puppet issues. I'll try to
get that sorted out.

--
Michael Mior
mm...@apache.org

2017-06-17 16:12 GMT-04:00 Julian Hyde :

> Many thanks.
>
> Yes, there are several PRs waiting for the fix to the VM.
>
> On Fri, Jun 16, 2017 at 11:07 AM, Jesus Camacho Rodriguez
>  wrote:
> > Hi Julian,
> >
> > It slipped my mind to reply. Since I'll be acting as RM for 1.13, it
> makes sense that I monitor the PRs and respond to them.
> >
> > I will also try to fix the issue with the Druid VM before the release.
> >
> > Thanks,
> > Jesús
> >
> >
> >
> >
> >
> > On 6/16/17, 7:00 PM, "Julian Hyde"  wrote:
> >
> >>Anyone?
> >>
> >>On Thu, Jun 15, 2017 at 9:32 AM, Julian Hyde  wrote:
> >>> I’m going to be out for the next week or so. Can someone please
> volunteer to monitor the pull requests? I strive to give each pull request
> a response within 24 hours, and commit them when they’re ready.
> >>>
> >>> Slow responses to pull requests are very discouraging to potential
> contributors, so this is a critical task for our project.
> >>>
> >>> Julian
> >>>
> >>
>

Re: Pull requests

[Julian Hyde]

Many thanks.

Yes, there are several PRs waiting for the fix to the VM.

On Fri, Jun 16, 2017 at 11:07 AM, Jesus Camacho Rodriguez
 wrote:
> Hi Julian,
>
> It slipped my mind to reply. Since I'll be acting as RM for 1.13, it makes 
> sense that I monitor the PRs and respond to them.
>
> I will also try to fix the issue with the Druid VM before the release.
>
> Thanks,
> Jesús
>
>
>
>
>
> On 6/16/17, 7:00 PM, "Julian Hyde"  wrote:
>
>>Anyone?
>>
>>On Thu, Jun 15, 2017 at 9:32 AM, Julian Hyde  wrote:
>>> I’m going to be out for the next week or so. Can someone please volunteer 
>>> to monitor the pull requests? I strive to give each pull request a response 
>>> within 24 hours, and commit them when they’re ready.
>>>
>>> Slow responses to pull requests are very discouraging to potential 
>>> contributors, so this is a critical task for our project.
>>>
>>> Julian
>>>
>>

Re: Pull requests

[Jesus Camacho Rodriguez]

Hi Julian,

It slipped my mind to reply. Since I'll be acting as RM for 1.13, it makes 
sense that I monitor the PRs and respond to them.

I will also try to fix the issue with the Druid VM before the release.

Thanks,
Jesús





On 6/16/17, 7:00 PM, "Julian Hyde"  wrote:

>Anyone?
>
>On Thu, Jun 15, 2017 at 9:32 AM, Julian Hyde  wrote:
>> I’m going to be out for the next week or so. Can someone please volunteer to 
>> monitor the pull requests? I strive to give each pull request a response 
>> within 24 hours, and commit them when they’re ready.
>>
>> Slow responses to pull requests are very discouraging to potential 
>> contributors, so this is a critical task for our project.
>>
>> Julian
>>
>

Re: Pull requests

[Julian Hyde]

Anyone?

On Thu, Jun 15, 2017 at 9:32 AM, Julian Hyde  wrote:
> I’m going to be out for the next week or so. Can someone please volunteer to 
> monitor the pull requests? I strive to give each pull request a response 
> within 24 hours, and commit them when they’re ready.
>
> Slow responses to pull requests are very discouraging to potential 
> contributors, so this is a critical task for our project.
>
> Julian
>