asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dmitry Lychagin (Code Review)" <do-not-re...@asterixdb.incubator.apache.org>
Subject Change in asterixdb[master]: [ASTERIXDB-2153][COMP][RT] Ensure the fulltext search option...
Date Thu, 02 Nov 2017 01:36:15 GMT
Dmitry Lychagin has posted comments on this change.

Change subject: [ASTERIXDB-2153][COMP][RT] Ensure the fulltext search option is properly handled
......................................................................


Patch Set 3:

(2 comments)

https://asterix-gerrit.ics.uci.edu/#/c/2116/3/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/FullTextContainsParameterCheckRule.java
File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/FullTextContainsParameterCheckRule.java:

Line 108:         if (fi == AlgebricksBuiltinFunctions.AND || fi == AlgebricksBuiltinFunctions.OR)
{
What about other cases? nested AND/OR, ftcontains() as input to other functions (not(ftcontains()),
etc). What about ftcontains() in other operators (not just SELECT/JOIN), for instance in ASSIGN?

This fix seems to be solving a narrow usecase but the problem would still remain in other
cases where ftcontains() is inside operators/expressions not matching this rule. The 3rd argument
would then not be rewritten from a record into name/value argument pairs. 

The right fix should go through the whole expression tree inside each operator and rewrite
all ftcontains() functions it encounters.
ILogicalOperator.acceptExpressionTransform() can do this.

I guess I can approve this fix if it's urgent, but we need a better solution long-term.


Line 123:             transformed = transformed || checkParameterForFuncExpr(op, funcExpr,
context, fi);
> BLOCKER SonarQube violation:
'transformed' is always 'false' at this point, so this line could be simplified to 'transformed
= checkParameterForFuncExpr(...)'


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I240fbe08891d29532c6fcd60638a3b6bbe8da771
Gerrit-PatchSet: 3
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: Taewoo Kim <wangsaeu@gmail.com>
Gerrit-Reviewer: Anon. E. Moose #1000171
Gerrit-Reviewer: Dmitry Lychagin <dmitry.lychagin@couchbase.com>
Gerrit-Reviewer: Ian2 Maxon <imaxon@uci.edu>
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Gerrit-HasComments: Yes

Mime
View raw message