impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-1286: Extract common conjuncts from disjunctions.
Date Tue, 08 Nov 2016 19:58:46 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-1286: Extract common conjuncts from disjunctions.
......................................................................


Patch Set 5: Code-Review+1

I was less concerned with what the output type is and more about how it is obtained. The problem
is that we don't have any comprehensible rules about how the output type of an expression
is obtained or what the input to the type derivation algorithm even is. In my mind a query
option shouldn't be part of the input to the type derivation. 

It's all a consequence of a pre-existing design so I'm not saying you should fix it as part
of this change.

I think the original sin is that the output type of an operation should be determined by the
input types, because that gives you a reasonable way to trace back and understand how the
type of an expression was derived. This isn't the case today since it looks at other information
and maybe depends in complicate way on other rewrites.

My point about dependent types is that you can achieve what you're arguing for without making
typechecking depend on a query optimisation. E.g. the logical types would be "LITERAL(1) +
LITERAL(1) => LITERAL(2)" instead of "TINYINT + TINYINT => SMALLINT) and they would
be lowered at some point into actual physical types. The current system of typing literals
seems to be trying to approximate this.

I'm not saying we shouldn't go ahead, but I think there are design problems with the type
system and I think this is digging the hole a little deeper. I'm ok as long as we have a way
to replace it with a more principled approach later without breaking things. It seems like
it's mostly corner cases that would be affected in the meantime and there probably is a way
out.

-- 
To view, visit http://gerrit.cloudera.org:8080/4877
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3cf9b950afaa3fd753d1b09ba5e540b5258940ad
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bharathv@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: No

Mime
View raw message