drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jinfeng Ni <jinfengn...@gmail.com>
Subject Re: Getting back on Calcite master: only a few steps left
Date Thu, 23 Jun 2016 06:11:25 GMT
Hi All,

It has been a while since we had discussion about getting Drill back
to Calcite master. We have made good progress by merging patches to
Calcite master. Thanks everyone who helped on this effort!

I took a look at the spreadsheet [1], and marked the patches based on
what I understand. So far, there are 4 patches marked as "TODO" (in
red), 2 patches marked as "PR pending review/merge" (in yellow).

1.  Can people of those 4 patches (Julian Le Dam, Jacques, Venki,
Aman) see if the patches have been merged to Calcite master (I may
miss something)?
2.  Once the necessary patches are in Calcite master, we probably need
have a discussion about the plan to do the REAL rebasing work.

Thoughts?

[1]  https://docs.google.com/spreadsheets/d/1Z0J5aMCBfqq_9SEl-LXsi_B8Ahaosygqke4cH6pPwmo/edit#gid=0

On Wed, Apr 20, 2016 at 1:04 PM, Julian Hyde <jhyde.apache@gmail.com> wrote:
> 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