-
Notifications
You must be signed in to change notification settings - Fork 96
Lens 1381 #20
base: master
Are you sure you want to change the base?
Conversation
@@ -39,6 +40,8 @@ | |||
private final Set<String> storageTables = new LinkedHashSet<String>(); | |||
@Getter | |||
private final UpdatePeriod period; | |||
|
|||
//TODO union : this is never set . Do we need this ?s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is used in lookahead.
Date getStartTime(); | ||
|
||
/** | ||
* End Time for this candidate (calculated based on schema) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope this is exclusive? in line with our usage of half-open ranges everywhere.
candidateSet.add(CandidateUtil.cloneStorageCandidate(sc)); | ||
continue; | ||
} else if (CandidateUtil.isPartiallyValidForTimeRanges(sc, cubeql.getTimeRanges())) { | ||
allCandidatesPartiallyValid.add(CandidateUtil.cloneStorageCandidate(sc)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of a util method, I think a copy constructor would be better.
return candidateSet ; | ||
} | ||
|
||
private boolean isMeasureAnswerablebyUnionCandidate(QueriedPhraseContext msr, Candidate uc, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function name is misleading because it's not always operating on union candidate. It's operating on any candidate. Secondly, the if instanceof
idioms can be avoided by using polymorphism.
* @return | ||
* @throws LensException | ||
*/ | ||
public Set<String> getTimePartitionCols(StorageCandidate candidate, CubeMetastoreClient metastoreClient) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not static like other methods?
* @param targetAst | ||
* @throws LensException | ||
*/ | ||
public static void copyASTs(QueryAST sourceAst, QueryAST targetAst) throws LensException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a thought: Should this method be in QueryAST
class seeing as it's doing all operations on the targetAst
.
Iterator<Candidate> itr = candidates.iterator(); | ||
while (itr.hasNext()) { | ||
if (itr.next().contains(filterCandidate)) { | ||
prunedCandidates.add(itr.next()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a bug. Calling itr.next
multiple times in one iteration.
private QueryAST queryAST; | ||
private CubeQueryContext cubeql; | ||
|
||
public JoinCandidate(Candidate childCandidate1, Candidate childCandidate2, CubeQueryContext cubeql) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any particular reason why we are limiting joins to 2 candidates and not allowing an arbitrary number of candidates to be joined.
…d duplicate projections
…s containing dimensions
No description provided.