[jira] [Comment Edited] (PHOENIX-4884) INSTR function should work seamlessly with literal and non-literal arguments

["James Taylor (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/PHOENIX-4884?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16598977#comment-16598977 ] 

James Taylor edited comment on PHOENIX-4884 at 8/31/18 4:33 PM:
----------------------------------------------------------------

Thanks, [~elserj]. Here's some feeback:

Just FYI, this is the correct way of doing it:
{code:java}
+            // We need something non-empty to search against (TODO do we?)
+            if (ptr.getLength() == 0) {
+              ptr.set(ByteUtil.EMPTY_BYTE_ARRAY);
+              return true;
+            }
{code}
Here in evaluate, if child.evaluate() returns true, that means that ptr is pointing at the value, so instead of this:
{code:java}
+            sourceStr = (String) PVarchar.INSTANCE.toObject(ptr, getChildren().get(0).getSortOrder());
{code}
do this:
{code:java}
sourceStr = (String)child.toObject(ptr, child.getSortOrder());{code}
Further down, if the evaluation of the second child returns false, you want to return false (this means "we don't have enough information to figure this out yet"). Otherwise, just use ptr to create the string object:
{code:java}
+        // A literal was not provided, try to evaluate the expression to a literal
+        if (searchStr == null){
+            Expression child = getChildren().get(1);
+            // Failed to compute the expression to a literal value
+            if (!child.evaluate(tuple, ptr)) {
+              return false;
+            }
+            searchStr = (String) child.toObject(ptr,child.getSortOrder());

{code}
Minor nit: no need for the toString here:
{code:java}
+        if (expr instanceof LiteralExpression) {
+            Object strValue = ((LiteralExpression) expr).getValue();
+            if (strValue != null) {
+                return strValue.toString();
{code}
You can just do this instead:
{code:java}
String strValue = (String)((LiteralExpression) expr).getValue();{code}
 


was (Author: jamestaylor):
Thanks, [~elserj]. Here's some feeback:

Just FYI, this is the correct way of doing it:
{code:java}
+            // We need something non-empty to search against (TODO do we?)
+            if (ptr.getLength() == 0) {
+              ptr.set(ByteUtil.EMPTY_BYTE_ARRAY);
+              return true;
+            }
{code}
Here in evaluate, if child.evaluate() returns true, that means that ptr is pointing at the value, so instead of this:
{code:java}
+            sourceStr = (String) PVarchar.INSTANCE.toObject(ptr, getChildren().get(0).getSortOrder());
{code}
do this:
{code:java}
sourceStr = (String)child.toObject(ptr, child.getSortOrder());{code}
Further down, if the evaluation of the second child returns false, you want to return false (this means "we don't have enough information to figure this out yet). Otherwise, just use ptr to create the string object:
{code:java}
+        // A literal was not provided, try to evaluate the expression to a literal
+        if (searchStr == null){
+            Expression child = getChildren().get(1);
+            // Failed to compute the expression to a literal value
+            if (!child.evaluate(tuple, ptr)) {
+              return false;
+            }
+            searchStr = (String) child.toObject(ptr,child.getSortOrder());

{code}
Minor nit: no need for the toString here:
{code:java}
+        if (expr instanceof LiteralExpression) {
+            Object strValue = ((LiteralExpression) expr).getValue();
+            if (strValue != null) {
+                return strValue.toString();
{code}
You can just do this instead:
{code:java}
String strValue = (String)((LiteralExpression) expr).getValue();{code}
 

> INSTR function should work seamlessly with literal and non-literal arguments
> ----------------------------------------------------------------------------
>
>                 Key: PHOENIX-4884
>                 URL: https://issues.apache.org/jira/browse/PHOENIX-4884
>             Project: Phoenix
>          Issue Type: Bug
>            Reporter: Josh Elser
>            Assignee: Josh Elser
>            Priority: Major
>             Fix For: 4.15.0, 5.1.0
>
>         Attachments: PHOENIX-4884.001.patch
>
>
> INSTR's documentation reads as though it should support an expression or a literal for either argument. At least, it doesn't say that it only supports one or the other.
> However, the implementation only handles the case of {{INSTR(expr, literal)}}. We can pretty easily make this better and work with any combination:
> e.g. {{INSTR(literal, expr)}}, {{INSTR(expr, expr)}}, {{INSTR(literal, literal)}}



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)