drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Julian Hyde <jhyde.apa...@gmail.com>
Subject Re: Getting back on Calcite master: only a few steps left
Date Wed, 20 Apr 2016 20:04:30 GMT
Regarding https://issues.apache.org/jira/browse/CALCITE-1150, "Create the a new DynamicRecordType,
avoiding star expansion when working with this type”. This feature will be useful, and as
I have said to Jacques would fit well within Calcite, but it’s a bit shapeless to me right
now. I would like to see some validator tests (any maybe one or two sql-to-rel converter tests)
so I get a feel for how it would work. When you have some tests can you post them in the JIRA
case? I don’t think you should charge ahead with the implementation because I might not
agree with the specification (i.e. the test cases).

I’l make the same comment in the JIRA case, and let’s continue the discussion there.

Julian



> On Apr 19, 2016, at 9:51 AM, Jinfeng Ni <jinfengni99@gmail.com> wrote:
> 
> @Jacques,
> 
> Sorry for the delay. Let me spend couple of days this week to get
> CALCITE-1150 back on track. Initially, I encountered one conflicting
> change in Calcite master, and broke couple of unit tests. If I can not
> get them solved, I'll ping you or Minji for a discussion.
> 
> 
> 
> On Mon, Apr 18, 2016 at 5:31 PM, Jacques Nadeau <jacques@dremio.com> wrote:
>> Hey All,
>> 
>> Following up to get a status update. We made some good initial progress but
>> it seems like people may have hit some challenges (or distractions). Can
>> everyone report on how they are doing?
>> 
>> Jinfeng, how are tests for CALCITE-1150 going? Can Minji help get together
>> test cases for CALCITE-1150? Maybe you could provide guidance on the set of
>> queries to test?
>> 
>> thanks,
>> Jacques
>> 
>> 
>> --
>> Jacques Nadeau
>> CTO and Co-Founder, Dremio
>> 
>> On Thu, Mar 31, 2016 at 4:19 PM, Julian Hyde <jhyde@apache.org> wrote:
>> 
>>> I’ve closed 1149, if we don’t need the feature.
>>> 
>>> Yes, we need a unit test for 1151. I offered a suggestion how.
>>> 
>>>> On Mar 31, 2016, at 11:59 AM, Sudheesh Katkam <skatkam@maprtech.com>
>>> wrote:
>>>> 
>>>> I submitted a patch for CALCITE-1151 <
>>> https://issues.apache.org/jira/browse/CALCITE-1151> (with changes to
>>> resolve a checkstyle error). I am waiting for comments regarding the unit
>>> test.
>>>> 
>>>> I added a comment to CALCITE-1149 <
>>> https://issues.apache.org/jira/browse/CALCITE-1149> with the workaround
>>> being used.
>>>> 
>>>> Thank you,
>>>> Sudheesh
>>>> 
>>>>> On Mar 16, 2016, at 5:19 PM, Jacques Nadeau <jacques@dremio.com>
wrote:
>>>>> 
>>>>> Yes, I'm trying to work through the failing unit tests.
>>>>> 
>>>>> I merged your change.
>>>>> 
>>>>> In the future you can pick compare & create pull request on your
branch
>>> and
>>>>> then change the target repo from apache to mine.
>>>>> 
>>>>> thanks,
>>>>> Jacques
>>>>> 
>>>>> 
>>>>> --
>>>>> Jacques Nadeau
>>>>> CTO and Co-Founder, Dremio
>>>>> 
>>>>> On Wed, Mar 16, 2016 at 4:39 PM, Aman Sinha <amansinha@apache.org>
>>> wrote:
>>>>> 
>>>>>> Jacques, I wasn't sure how to create a pull request against your
>>> branch;
>>>>>> for  CALCITE-1108 you can cherry-pick from here:
>>>>>> 
>>>>>> 
>>> https://github.com/amansinha100/incubator-calcite/commits/calcite-drill-2
>>>>>> 
>>>>>> BTW,  there are unit test failures on your branch which I assume
is
>>>>>> expected for now ?
>>>>>> 
>>>>>> On Tue, Mar 15, 2016 at 6:56 PM, Jacques Nadeau <jacques@dremio.com>
>>>>>> wrote:
>>>>>> 
>>>>>>> Why don't you guys propose patches for my branch and I'll incorporate
>>>>>> until
>>>>>>> we get to a good state. Once we feel good about it, I'll clean
up the
>>>>>>> revision history.
>>>>>>> 
>>>>>>> --
>>>>>>> Jacques Nadeau
>>>>>>> CTO and Co-Founder, Dremio
>>>>>>> 
>>>>>>> On Tue, Mar 15, 2016 at 11:01 AM, Jinfeng Ni <jinfengni99@gmail.com>
>>>>>>> wrote:
>>>>>>> 
>>>>>>>> I'll add test for CALCITE-1150.
>>>>>>>> 
>>>>>>>> 
>>>>>>>> 
>>>>>>>> On Tue, Mar 15, 2016 at 9:45 AM, Sudheesh Katkam <
>>> skatkam@maprtech.com
>>>>>>> 
>>>>>>>> wrote:
>>>>>>>>> CALCITE-1149 [Extend CALCITE-845] <
>>>>>>>> 
>>>>>>> 
>>>>>> 
>>> https://github.com/mapr/incubator-calcite/commit/bd73728a8297e15331ae956096eab0e15bbbbb3f
>>>>>>>> 
>>>>>>>> does not need to be committed into Calcite. DRILL-4372 <
>>>>>>>> https://issues.apache.org/jira/browse/DRILL-4372> supersedes
that
>>>>>> patch.
>>>>>>>>> 
>>>>>>>>> I will add a test case for CALCITE-1151.
>>>>>>>>> 
>>>>>>>>> Thank you,
>>>>>>>>> Sudheesh
>>>>>>>>> 
>>>>>>>>>> On Mar 15, 2016, at 9:04 AM, Aman Sinha <amansinha@apache.org>
>>>>>> wrote:
>>>>>>>>>> 
>>>>>>>>>> I'll add a test for CALCITE-1108.   For 1105 I am
not yet sure but
>>>>>>> will
>>>>>>>>>> look through the old drill commits to see what test
was added
>>> there.
>>>>>>>>>> 
>>>>>>>>>> On Sun, Mar 13, 2016 at 11:15 PM, Minji Kim <minji@dremio.com>
>>>>>> wrote:
>>>>>>>>>> 
>>>>>>>>>>> I will add more test cases to CALCITE-1148 in
addition to the ones
>>>>>>>> already
>>>>>>>>>>> there.  I noticed a few more problems while testing
the patch
>>>>>> against
>>>>>>>> drill
>>>>>>>>>>> master.  I am still working through these issues,
so I will add
>>>>>> more
>>>>>>>> test
>>>>>>>>>>> cases as I find/fix them.  -Minji
>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>>> On 3/13/16 10:54 PM, Jacques Nadeau wrote:
>>>>>>>>>>> 
>>>>>>>>>>>> Hey All,
>>>>>>>>>>>> 
>>>>>>>>>>>> I've been working on rebasing and tracking
all the necessary
>>>>>> commits
>>>>>>>> that
>>>>>>>>>>>> are on the Drill Calcite fork so that we
can get back onto
>>> master.
>>>>>>> The
>>>>>>>>>>>> current working branch is here: [1]. It includes
the following
>>>>>>> commits
>>>>>>>>>>>> 
>>>>>>>>>>>> [CALCITE-1148] Fix RelTrait conversion (e.g.
distribution,
>>>>>>> collation),
>>>>>>>>>>>> added test cases. (Minji Kim) #77def4a
>>>>>>>>>>>> [CALCITE-991] Create separate FunctionCategories
for table
>>>>>> functions
>>>>>>>> and
>>>>>>>>>>>> macros (Julien Le Dem) #b1c203d
>>>>>>>>>>>> [CALCITE-1149] Derive AVG’s return type
by a customizable policy
>>>>>>>> (Sudheesh
>>>>>>>>>>>> Katkam) #18882cd
>>>>>>>>>>>> [CALCITE-1151] Overriding the SqlSpecialOperator#createCall
>>> method
>>>>>>>> given
>>>>>>>>>>>> the usage by CompoundIdentifierConverter
(Sudheesh Katkam)
>>>>>> #2320c7f
>>>>>>>>>>>> [CALCITE-1108] Don't use 'SumEmptyIsZero'
(SUM0) window aggregate
>>>>>>>> until
>>>>>>>>>>>> CALCITE-777 is fixed. (Aman Sinha) #13466fa
>>>>>>>>>>>> [CALCITE-1107] Make SqlSumEmptyIsZeroAggFunction
constructor
>>>>>> public.
>>>>>>>>>>>> (Jinfeng Ni) #b6c3178
>>>>>>>>>>>> [CALCITE-1106] Expose Constructor for ProjectJoinTransposeRule.
>>>>>>> (Aman
>>>>>>>>>>>> Sinha) #d169c37
>>>>>>>>>>>> [CALCITE-1105] Add return type-inference
strategy for arithmetic
>>>>>>>> operators
>>>>>>>>>>>> when one of the arguments is ANY type. (Aman
Sinha) #df818c9
>>>>>>>>>>>> [CALCITE-1150] Add DynamicRecordType and
the concept of
>>> unresolved
>>>>>>>> star
>>>>>>>>>>>> (Jinfeng Ni) #29c7771
>>>>>>>>>>>> [CALCITE-1152] Small ANY type fixes (Mehant
Baid) #31efdda
>>>>>>>>>>>> [CALCITE-528] Ensure uniquification is done
in a case aware way
>>>>>>>> according
>>>>>>>>>>>> to type system and catalog policies. (Jacques
Nadeau) #5a3d854
>>>>>>>>>>>> 
>>>>>>>>>>>> Many commits, listed below, don't have tests
right now so I'd
>>> like
>>>>>>> to
>>>>>>>> get
>>>>>>>>>>>> people to raise their hand and work on tests
for each of the
>>>>>>> commits.
>>>>>>>>>>>> 
>>>>>>>>>>>> [CALCITE-991] Create separate FunctionCategories
for table
>>>>>> functions
>>>>>>>> and
>>>>>>>>>>>> macros (Julien Le Dem) #b1c203d
>>>>>>>>>>>> [CALCITE-1149] Derive AVG’s return type
by a customizable policy
>>>>>>>> (Sudheesh
>>>>>>>>>>>> Katkam) #18882cd
>>>>>>>>>>>> [CALCITE-1151] Overriding the SqlSpecialOperator#createCall
>>> method
>>>>>>>> given
>>>>>>>>>>>> the usage by CompoundIdentifierConverter
(Sudheesh Katkam)
>>>>>> #2320c7f
>>>>>>>>>>>> [CALCITE-1108] Don't use 'SumEmptyIsZero'
(SUM0) window aggregate
>>>>>>>> until
>>>>>>>>>>>> CALCITE-777 is fixed. (Aman Sinha) #13466fa
>>>>>>>>>>>> [CALCITE-1105] Add return type-inference
strategy for arithmetic
>>>>>>>> operators
>>>>>>>>>>>> when one of the arguments is ANY type. (Aman
Sinha) #df818c9
>>>>>>>>>>>> [CALCITE-1150] Add DynamicRecordType and
the concept of
>>> unresolved
>>>>>>>> star
>>>>>>>>>>>> (Jinfeng Ni) #29c7771
>>>>>>>>>>>> [CALCITE-1152] Small ANY type fixes (Mehant
Baid) #31efdda
>>>>>>>>>>>> [CALCITE-528] Ensure uniquification is done
in a case aware way
>>>>>>>> according
>>>>>>>>>>>> to type system and catalog policies. (Jacques
Nadeau) #5a3d854
>>>>>>>>>>>> 
>>>>>>>>>>>> Also note that there are currently 15 tests
failing in this
>>>>>> Calcite
>>>>>>>> branch
>>>>>>>>>>>> that I haven't yet tracked down.
>>>>>>>>>>>> 
>>>>>>>>>>>> org.apache.calcite.test.SqlToRelConverterTest
(10 tests)
>>>>>>>>>>>> org.apache.calcite.test.JdbcTest (2 tests)
>>>>>>>>>>>> org.apache.calcite.test.RelOptRulesTest.txt
(1 test)
>>>>>>>>>>>> org.apache.calcite.test.SqlValidatorTest.txt
(1 test)
>>>>>>>>>>>> org.apache.calcite.rel.rel2sql.RelToSqlConverterTest
(1 test)
>>>>>>>>>>>> 
>>>>>>>>>>>> Note that I also reworked the Schema changes
items so that they
>>>>>>> don't
>>>>>>>> have
>>>>>>>>>>>> any impact on code paths unless the system
returns a
>>>>>>>> DynamicRecordType.
>>>>>>>>>>>> Once we get these changes looking good, we
can move to making
>>>>>> small
>>>>>>>>>>>> modifications in the Drill codebase to use
this new record type.
>>>>>>>>>>>> 
>>>>>>>>>>>> Can people raise their hands to confirm they
will be able to
>>> write
>>>>>>>> tests
>>>>>>>>>>>> cases for issues they own?
>>>>>>>>>>>> 
>>>>>>>>>>>> thanks,
>>>>>>>>>>>> Jacques
>>>>>>>>>>>> 
>>>>>>>>>>>> [1]
>>>>>>>> https://github.com/jacques-n/incubator-calcite/tree/calcite-drill-2
>>>>>>>>>>>> 
>>>>>>>>>>>> --
>>>>>>>>>>>> Jacques Nadeau
>>>>>>>>>>>> CTO and Co-Founder, Dremio
>>>>>>>>>>>> 
>>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>> 
>>>>>>>> 
>>>>>>> 
>>>>>> 
>>>> 
>>> 
>>> 


Mime
View raw message