Jianfeng Jia has posted comments on this change.

Change subject: Change logical plan to apply filter from 2ndary index
......................................................................


Patch Set 12:

(27 comments)

https://asterix-gerrit.ics.uci.edu/#/c/1727/11/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/algebra/operators/physical/BTreeSearchPOperator.java
File 
asterixdb/asterix-algebra/src/main/java/org/apache/asterix/algebra/operators/physical/BTreeSearchPOperator.java:

PS11, Line 118: propagateIndexFilter();
> getPropageIndexFilter -> propagateIndexFilter
Done


https://asterix-gerrit.ics.uci.edu/#/c/1727/11/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/algebra/operators/physical/IndexSearchPOperator.java
File 
asterixdb/asterix-algebra/src/main/java/org/apache/asterix/algebra/operators/physical/IndexSearchPOperator.java:

PS11, Line 66: getScanVariables
> Can it be just as.getVariables()?
The filter values are appended if using IntroducingFilterRule. For the case of 
the scan, I think we should not deliver those filter variables.


https://asterix-gerrit.ics.uci.edu/#/c/1727/11/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/algebra/operators/physical/InvertedIndexPOperator.java
File 
asterixdb/asterix-algebra/src/main/java/org/apache/asterix/algebra/operators/physical/InvertedIndexPOperator.java:

PS11, Line 125: 
> It looks that propagateIndexFilter can be retrieved from unnestMapOp so you
Done


https://asterix-gerrit.ics.uci.edu/#/c/1727/11/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/IntroduceLSMComponentFilterRule.java
File 
asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/IntroduceLSMComponentFilterRule.java:

Line 226: 
> MAJOR SonarQube violation:
it won't be as clear as it of now.


PS11, Line 244: (Unnest
> Generalize the filter propagation using a loop?  In other words, make the c
I just found an issue for the intersection case, which requires NOT to compare 
the filter field, but has to carry the value out of the pipeline. 

so I think the code has to be operator specific then?


PS11, Line 271: ersect.
> Throw an exception?
Done


PS11, Line 313: 
> ArrayList -> List
Done


PS11, Line 317: 
> cast is not needed:
Done


PS11, Line 319: computeA
> It should never be null?
Done


PS11, Line 328: ndantOp = queue.poll(
> error code
Done


Line 333:                     AbstractFunctionCallExpression f = 
(AbstractFunctionCallExpression) unnestExpr;
> MAJOR SonarQube violation:
it won't be as clear as it of now.


https://asterix-gerrit.ics.uci.edu/#/c/1727/11/asterixdb/asterix-app/src/test/resources/optimizerts/queries/filter/btree-btree-search-wo-query-filter.aql
File 
asterixdb/asterix-app/src/test/resources/optimizerts/queries/filter/btree-btree-search-wo-query-filter.aql:

PS11, Line 36:  in da
> write statement is not needed.
Done


https://asterix-gerrit.ics.uci.edu/#/c/1727/11/asterixdb/asterix-app/src/test/resources/optimizerts/queries/filter/btree-btree-search.aql
File 
asterixdb/asterix-app/src/test/resources/optimizerts/queries/filter/btree-btree-search.aql:

PS11, Line 36: time("
> write statement is not needed.
Done


https://asterix-gerrit.ics.uci.edu/#/c/1727/11/asterixdb/asterix-app/src/test/resources/optimizerts/queries/filter/inverted-btree-search-wo-query-filter.aql
File 
asterixdb/asterix-app/src/test/resources/optimizerts/queries/filter/inverted-btree-search-wo-query-filter.aql:

PS11, Line 36: 
> write statement is not needed.
Done


https://asterix-gerrit.ics.uci.edu/#/c/1727/11/asterixdb/asterix-app/src/test/resources/optimizerts/queries/filter/inverted-btree-search.aql
File 
asterixdb/asterix-app/src/test/resources/optimizerts/queries/filter/inverted-btree-search.aql:

PS11, Line 36: s_star
> write statement is not needed.
Done


https://asterix-gerrit.ics.uci.edu/#/c/1727/11/asterixdb/asterix-app/src/test/resources/optimizerts/queries/filter/multi-index-btree-search-wo-query-filter.aql
File 
asterixdb/asterix-app/src/test/resources/optimizerts/queries/filter/multi-index-btree-search-wo-query-filter.aql:

PS11, Line 38: rea
> write statement is not needed
Done


https://asterix-gerrit.ics.uci.edu/#/c/1727/11/asterixdb/asterix-app/src/test/resources/optimizerts/queries/filter/multi-index-btree-search.aql
File 
asterixdb/asterix-app/src/test/resources/optimizerts/queries/filter/multi-index-btree-search.aql:

PS11, Line 38: egion 
> write statement is not needed.
Done


https://asterix-gerrit.ics.uci.edu/#/c/1727/11/asterixdb/asterix-app/src/test/resources/optimizerts/queries/filter/rtree-btree-search-wo-query-filter.aql
File 
asterixdb/asterix-app/src/test/resources/optimizerts/queries/filter/rtree-btree-search-wo-query-filter.aql:

PS11, Line 36: egion 
> write statement is not needed
Done


https://asterix-gerrit.ics.uci.edu/#/c/1727/11/asterixdb/asterix-app/src/test/resources/optimizerts/queries/filter/rtree-btree-search.aql
File 
asterixdb/asterix-app/src/test/resources/optimizerts/queries/filter/rtree-btree-search.aql:

PS11, Line 36: egion 
> write statement is not needed
Done


https://asterix-gerrit.ics.uci.edu/#/c/1727/11/asterixdb/asterix-app/src/test/resources/optimizerts/results/filter/btree-btree-search-wo-query-filter.plan
File 
asterixdb/asterix-app/src/test/resources/optimizerts/results/filter/btree-btree-search-wo-query-filter.plan:

PS11, Line 9: ONE_TO_ONE_EXCHANGE
> Is the physical plan here different with vs. without filter?
They are the same. 
I tried to override the toString method by detecting the filter. However, the 
filter value is decided when it calls `contributeRuntimeOperator` which is 
after we print this plan. 

Basically, it's a parameter of the BTREE_SEARCH. We did print that value in 
full plan which contains the operator information. Will it be OK?


https://asterix-gerrit.ics.uci.edu/#/c/1727/11/asterixdb/asterix-app/src/test/resources/runtimets/queries/filters/delete/delete.11.ddl.aql
File 
asterixdb/asterix-app/src/test/resources/runtimets/queries/filters/delete/delete.11.ddl.aql:

PS11, Line 20: 
> The last drop dataverse is not needed because after each test, the test exe
Done


https://asterix-gerrit.ics.uci.edu/#/c/1727/11/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/MetadataProvider.java
File 
asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/MetadataProvider.java:

PS11, Line 430:  boolean propagateFilter
> Is the boolean info somehow can be obtained from Dataset dataset, since fil
For the purpose of specifying if a filter exists, we can get it from the 
dataset for sure. However, whether we should propagate it depends on the query, 
I think. E.g., if there is no secondary-to-primary lookup case, then we don't 
need to propagate it.


PS11, Line 476: propagateFilter
> Is the boolean info somehow can be obtained from Dataset dataset, since fil
same reason as before :-)


https://asterix-gerrit.ics.uci.edu/#/c/1727/11/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/AbstractScanOperator.java
File 
hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/AbstractScanOperator.java:

PS11, Line 40: ;
> the method is the same as getVariables()?
For the scan operator case, yes. 
In the case of UnnestMap, this method will detect if filter value is appended.


https://asterix-gerrit.ics.uci.edu/#/c/1727/11/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/AbstractUnnestMapOperator.java
File 
hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/AbstractUnnestMapOperator.java:

PS11, Line 113: markPropagageIndexFilter() {
> setPropagateIndexFilter(boolean propagateIndexFilter) -> markPropagageIndex
Done


PS11, Line 117: propagateIndexFilter() 
> getPropagateIndexFilter() -> propagateFilter()
Done


https://asterix-gerrit.ics.uci.edu/#/c/1727/11/hyracks-fullstack/hyracks/hyracks-examples/hyracks-integration-tests/src/test/java/org/apache/hyracks/tests/am/rtree/AbstractRTreeOperatorTest.java
File 
hyracks-fullstack/hyracks/hyracks-examples/hyracks-integration-tests/src/test/java/org/apache/hyracks/tests/am/rtree/AbstractRTreeOperatorTest.java:

PS11, Line 26: .IB
> expand imports
Done


-- 
To view, visit https://asterix-gerrit.ics.uci.edu/1727
To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0e2fe0208662e5dcd49d1a22bfb58f96533e9497
Gerrit-PatchSet: 12
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: Jianfeng Jia <[email protected]>
Gerrit-Reviewer: Ian Maxon <[email protected]>
Gerrit-Reviewer: Jenkins <[email protected]>
Gerrit-Reviewer: Jianfeng Jia <[email protected]>
Gerrit-Reviewer: Luo Chen <[email protected]>
Gerrit-Reviewer: Taewoo Kim <[email protected]>
Gerrit-Reviewer: Yingyi Bu <[email protected]>
Gerrit-Reviewer: abdullah alamoudi <[email protected]>
Gerrit-HasComments: Yes

Reply via email to