phoenix-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "James Taylor (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (PHOENIX-3918) Ensure all function implementations handle null args correctly
Date Thu, 08 Jun 2017 23:58:18 GMT

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

James Taylor commented on PHOENIX-3918:
---------------------------------------

Thanks for the updated patch, [~tdsilva]. It's good that we identify built-in functions that
potentially are not handling null correctly. The patch doesn't look right, though. Let me
try to explain the difference between returning false versus true and handling null:
- a function should only return false if any child expressions return false. This means "I
don't have enough information to calculate a result". This can really only happen when executing
on the server side during filter evaluation. In this case, the expression evaluation is only
seeing partial state: essentially each Cell is fed into the expression and an attempt is made
to evaluate it. For example {{WHERE A + B < 5}} might see the Cell for A first, but not
yet have seen B, so false would be returned for the + expression and subsequently by the <
expression. Once B is seen, then the expression can be evaluated.
- in the case that a child returns true, it may have been evaluated to null. This is the case
when ptr.getLength() == 0. When a child returns a value, it will always return the same same
value, so there's no need to continue evaluating it again and again. There are compound expressions
such as AND and OR that take advantage of this. If false is returned, though, these compound
expressions would be evaluated again and again. So this code isn't correct:
{code}
-            if (!offsetExpr.evaluate(tuple, ptr)) return false;
+            if (!offsetExpr.evaluate(tuple, ptr) || ptr.getLength() == 0) return false;
{code}
- For most (but not all) built-in functions, when null is encountered (i.e. a child expression
evaluated, returning true, and ptr.getLength()==0), then the function can immediately return
null. So most often, the code will look like this:
{code}
    if (!offsetExpr.evaluate(tuple, ptr)) return false;
    if (ptr.getLength() == 0) return true;
{code}
- The exception are expressions like || and ARRAY_CAT which combines children together simply
skipping null children.

> Ensure all function implementations handle null args correctly
> --------------------------------------------------------------
>
>                 Key: PHOENIX-3918
>                 URL: https://issues.apache.org/jira/browse/PHOENIX-3918
>             Project: Phoenix
>          Issue Type: Bug
>            Reporter: Samarth Jain
>            Assignee: Thomas D'Silva
>             Fix For: 4.12.0
>
>         Attachments: PHOENIX-3918.patch, PHOENIX-3918-v2.patch
>
>
> {code}
> testBothParametersNull(org.apache.phoenix.end2end.TimezoneOffsetFunctionIT)  Time elapsed:
2.272 sec  <<< ERROR!
> java.sql.SQLException: ERROR 201 (22000): Illegal data. Unknown timezone 
> 	at org.apache.phoenix.end2end.TimezoneOffsetFunctionIT.testBothParametersNull(TimezoneOffsetFunctionIT.java:130)
> timezoneParameterNull(org.apache.phoenix.end2end.TimezoneOffsetFunctionIT)  Time elapsed:
2.273 sec  <<< ERROR!
> java.sql.SQLException: ERROR 201 (22000): Illegal data. Unknown timezone 
> 	at org.apache.phoenix.end2end.TimezoneOffsetFunctionIT.timezoneParameterNull(TimezoneOffsetFunctionIT.java:151)
> dateParameterNull(org.apache.phoenix.end2end.TimezoneOffsetFunctionIT)  Time elapsed:
2.254 sec  <<< ERROR!
> java.sql.SQLException: ERROR 201 (22000): Illegal data. Expected length of at least 8
bytes, but had 0
> 	at org.apache.phoenix.end2end.TimezoneOffsetFunctionIT.dateParameterNull(TimezoneOffsetFunctionIT.java:172)
> {code}



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

Mime
View raw message