Uploaded image for project: 'Tajo'
  1. Tajo
  2. TAJO-182

Correct NULL value handling of primitive operators

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.8.0
    • Component/s: Planner/Optimizer
    • Labels:
      None

      Description

      If some domain value is compared to Null value, the current implementation will cause InvalidOperationException. Such cases should result in 'false'.

      If some domain value is compared to Null value, the current implementation will cause either InvalidOperationException or FALSE. Such cases should result in NULL.

      Many primitive operators including arithmetic and comparison do not consider three valued logic. This patch corrects this problem. The original issue title was 'Comparison of primitive values including null value should return NULL.' This issue was expanded for null value handling of all primitive operators.

      1. TAJO-182_2.patch
        76 kB
        Hyunsik Choi
      2. TAJO-182_3.patch
        78 kB
        Hyunsik Choi
      3. TAJO-182.patch
        29 kB
        Hyunsik Choi

        Issue Links

          Activity

          Hide
          prafulla Prafulla T added a comment -

          Has this issue been fixed in 3e6d684a9f1c7592c99d359c376956fcc9e917ba ?
          https://github.com/apache/incubator-tajo/commit/3e6d684a9f1c7592c99d359c376956fcc9e917ba

          Show
          prafulla Prafulla T added a comment - Has this issue been fixed in 3e6d684a9f1c7592c99d359c376956fcc9e917ba ? https://github.com/apache/incubator-tajo/commit/3e6d684a9f1c7592c99d359c376956fcc9e917ba
          Hide
          prafulla Prafulla T added a comment -

          Also I think that NULL comparison should be evaluated to NULL which in turn should be interpreted as false in most of cases.
          I checked the implementation and Tajo does not seem to be doing that. I may have missed somethings.

          Show
          prafulla Prafulla T added a comment - Also I think that NULL comparison should be evaluated to NULL which in turn should be interpreted as false in most of cases. I checked the implementation and Tajo does not seem to be doing that. I may have missed somethings.
          Hide
          hyunsik Hyunsik Choi added a comment -

          Hi Prafulla,

          Thank you for reminding this issue!! Yes, you are right. Exactly, null comparison should result in NULL value instead of false value. Actually, almost of all functions are doing that. But, operators between primitive values does not so. This issue was intended to an temporal solution. We have to fix this issue.

          Show
          hyunsik Hyunsik Choi added a comment - Hi Prafulla, Thank you for reminding this issue!! Yes, you are right. Exactly, null comparison should result in NULL value instead of false value. Actually, almost of all functions are doing that. But, operators between primitive values does not so. This issue was intended to an temporal solution. We have to fix this issue.
          Hide
          prafulla Prafulla T added a comment -

          Thanks, I am taking a look at this one.
          Does anyone know how to specify literal NULL in query?
          Following did not work.

          tajo> select id > NULL from table1;
          ERROR: syntax error at or near 'NULL'
          LINE 1:12 select id > NULL from table1
          ^^^^

          PS: ^^^^ is below NULL, Jira formatting is putting it at the beginning

          Show
          prafulla Prafulla T added a comment - Thanks, I am taking a look at this one. Does anyone know how to specify literal NULL in query? Following did not work. tajo> select id > NULL from table1; ERROR: syntax error at or near 'NULL' LINE 1:12 select id > NULL from table1 ^^^^ PS: ^^^^ is below NULL, Jira formatting is putting it at the beginning
          Hide
          hyunsik Hyunsik Choi added a comment -

          This is because SQL parser does not recognize null literal. Actually, this needs only trivial change, and I'll do that work soon.

          Show
          hyunsik Hyunsik Choi added a comment - This is because SQL parser does not recognize null literal. Actually, this needs only trivial change, and I'll do that work soon.
          Hide
          hyunsik Hyunsik Choi added a comment -

          Prafulla,

          I've attached the patch. If you can afford to review this patch, could you do that? This patch also includes the fix of NULL literal. I added the following unit tests.

              testSimpleEval("select null is null", new String[] {"t"});
              testSimpleEval("select null is not null", new String[] {"f"});
          
              testSimpleEval("select (1::int2 > null) is null", new String[] {"t"});
              testSimpleEval("select (1::int2 < null) is null", new String[] {"t"});
              testSimpleEval("select (1::int2 >= null) is null", new String[] {"t"});
              testSimpleEval("select (1::int2 <= null) is null", new String[] {"t"});
              testSimpleEval("select (1::int2 <> null) is null", new String[] {"t"});
          
              testSimpleEval("select (1::int4 > null) is null", new String[] {"t"});
              testSimpleEval("select (1::int4 < null) is null", new String[] {"t"});
              testSimpleEval("select (1::int4 >= null) is null", new String[] {"t"});
              testSimpleEval("select (1::int4 <= null) is null", new String[] {"t"});
              testSimpleEval("select (1::int4 <> null) is null", new String[] {"t"});
          
              testSimpleEval("select (1::int8 > null) is null", new String[] {"t"});
              testSimpleEval("select (1::int8 < null) is null", new String[] {"t"});
              testSimpleEval("select (1::int8 >= null) is null", new String[] {"t"});
              testSimpleEval("select (1::int8 <= null) is null", new String[] {"t"});
              testSimpleEval("select (1::int8 <> null) is null", new String[] {"t"});
          
              testSimpleEval("select (1::float > null) is null", new String[] {"t"});
              testSimpleEval("select (1::float < null) is null", new String[] {"t"});
              testSimpleEval("select (1::float >= null) is null", new String[] {"t"});
              testSimpleEval("select (1::float <= null) is null", new String[] {"t"});
              testSimpleEval("select (1::float <> null) is null", new String[] {"t"});
          
              testSimpleEval("select (1::float8 > null) is null", new String[] {"t"});
              testSimpleEval("select (1::float8 < null) is null", new String[] {"t"});
              testSimpleEval("select (1::float8 >= null) is null", new String[] {"t"});
              testSimpleEval("select (1::float8 <= null) is null", new String[] {"t"});
              testSimpleEval("select (1::float8 <> null) is null", new String[] {"t"});
          
              testSimpleEval("select ('abc' > null) is null", new String[] {"t"});
              testSimpleEval("select ('abc' < null) is null", new String[] {"t"});
              testSimpleEval("select ('abc' >= null) is null", new String[] {"t"});
              testSimpleEval("select ('abc' <= null) is null", new String[] {"t"});
              testSimpleEval("select ('abc' <> null) is null", new String[] {"t"});
          
              testSimpleEval("select ('1980-04-01'::date > null) is null", new String[] {"t"});
              testSimpleEval("select ('1980-04-01'::date < null) is null", new String[] {"t"});
              testSimpleEval("select ('1980-04-01'::date >= null) is null", new String[] {"t"});
              testSimpleEval("select ('1980-04-01'::date <= null) is null", new String[] {"t"});
              testSimpleEval("select ('1980-04-01'::date <> null) is null", new String[] {"t"});
          
              testSimpleEval("select ('09:08:50'::time > null) is null", new String[] {"t"});
              testSimpleEval("select ('09:08:50'::time < null) is null", new String[] {"t"});
              testSimpleEval("select ('09:08:50'::time >= null) is null", new String[] {"t"});
              testSimpleEval("select ('09:08:50'::time <= null) is null", new String[] {"t"});
              testSimpleEval("select ('09:08:50'::time <> null) is null", new String[] {"t"});
          
              testSimpleEval("select ('1980-04-01 01:50:30'::timestamp > null) is null", new String[] {"t"});
              testSimpleEval("select ('1980-04-01 01:50:30'::timestamp < null) is null", new String[] {"t"});
              testSimpleEval("select ('1980-04-01 01:50:30'::timestamp >= null) is null", new String[] {"t"});
              testSimpleEval("select ('1980-04-01 01:50:30'::timestamp <= null) is null", new String[] {"t"});
              testSimpleEval("select ('1980-04-01 01:50:30'::timestamp <> null) is null", new String[] {"t"});
          
          Show
          hyunsik Hyunsik Choi added a comment - Prafulla, I've attached the patch. If you can afford to review this patch, could you do that? This patch also includes the fix of NULL literal. I added the following unit tests. testSimpleEval( "select null is null " , new String [] { "t" }); testSimpleEval( "select null is not null " , new String [] { "f" }); testSimpleEval( "select (1::int2 > null ) is null " , new String [] { "t" }); testSimpleEval( "select (1::int2 < null ) is null " , new String [] { "t" }); testSimpleEval( "select (1::int2 >= null ) is null " , new String [] { "t" }); testSimpleEval( "select (1::int2 <= null ) is null " , new String [] { "t" }); testSimpleEval( "select (1::int2 <> null ) is null " , new String [] { "t" }); testSimpleEval( "select (1::int4 > null ) is null " , new String [] { "t" }); testSimpleEval( "select (1::int4 < null ) is null " , new String [] { "t" }); testSimpleEval( "select (1::int4 >= null ) is null " , new String [] { "t" }); testSimpleEval( "select (1::int4 <= null ) is null " , new String [] { "t" }); testSimpleEval( "select (1::int4 <> null ) is null " , new String [] { "t" }); testSimpleEval( "select (1::int8 > null ) is null " , new String [] { "t" }); testSimpleEval( "select (1::int8 < null ) is null " , new String [] { "t" }); testSimpleEval( "select (1::int8 >= null ) is null " , new String [] { "t" }); testSimpleEval( "select (1::int8 <= null ) is null " , new String [] { "t" }); testSimpleEval( "select (1::int8 <> null ) is null " , new String [] { "t" }); testSimpleEval( "select (1:: float > null ) is null " , new String [] { "t" }); testSimpleEval( "select (1:: float < null ) is null " , new String [] { "t" }); testSimpleEval( "select (1:: float >= null ) is null " , new String [] { "t" }); testSimpleEval( "select (1:: float <= null ) is null " , new String [] { "t" }); testSimpleEval( "select (1:: float <> null ) is null " , new String [] { "t" }); testSimpleEval( "select (1::float8 > null ) is null " , new String [] { "t" }); testSimpleEval( "select (1::float8 < null ) is null " , new String [] { "t" }); testSimpleEval( "select (1::float8 >= null ) is null " , new String [] { "t" }); testSimpleEval( "select (1::float8 <= null ) is null " , new String [] { "t" }); testSimpleEval( "select (1::float8 <> null ) is null " , new String [] { "t" }); testSimpleEval( "select ('abc' > null ) is null " , new String [] { "t" }); testSimpleEval( "select ('abc' < null ) is null " , new String [] { "t" }); testSimpleEval( "select ('abc' >= null ) is null " , new String [] { "t" }); testSimpleEval( "select ('abc' <= null ) is null " , new String [] { "t" }); testSimpleEval( "select ('abc' <> null ) is null " , new String [] { "t" }); testSimpleEval( "select ('1980-04-01'::date > null ) is null " , new String [] { "t" }); testSimpleEval( "select ('1980-04-01'::date < null ) is null " , new String [] { "t" }); testSimpleEval( "select ('1980-04-01'::date >= null ) is null " , new String [] { "t" }); testSimpleEval( "select ('1980-04-01'::date <= null ) is null " , new String [] { "t" }); testSimpleEval( "select ('1980-04-01'::date <> null ) is null " , new String [] { "t" }); testSimpleEval( "select ('09:08:50'::time > null ) is null " , new String [] { "t" }); testSimpleEval( "select ('09:08:50'::time < null ) is null " , new String [] { "t" }); testSimpleEval( "select ('09:08:50'::time >= null ) is null " , new String [] { "t" }); testSimpleEval( "select ('09:08:50'::time <= null ) is null " , new String [] { "t" }); testSimpleEval( "select ('09:08:50'::time <> null ) is null " , new String [] { "t" }); testSimpleEval( "select ('1980-04-01 01:50:30'::timestamp > null ) is null " , new String [] { "t" }); testSimpleEval( "select ('1980-04-01 01:50:30'::timestamp < null ) is null " , new String [] { "t" }); testSimpleEval( "select ('1980-04-01 01:50:30'::timestamp >= null ) is null " , new String [] { "t" }); testSimpleEval( "select ('1980-04-01 01:50:30'::timestamp <= null ) is null " , new String [] { "t" }); testSimpleEval( "select ('1980-04-01 01:50:30'::timestamp <> null ) is null " , new String [] { "t" });
          Hide
          prafulla Prafulla T added a comment - - edited

          Sure,
          Here are my review comments.

          Float4Datum::compareTo should raise exception when NullValue is seen? Just to be safe?
          I see that we can do this in almost all other classes Int,Text etc.

          Type of a > NULL should be Boolean OR NULL ? If NULL, we need to change type determination logic?

          For NULL with AND /OR, we need to consider three value logic?
          https://en.wikipedia.org/wiki/Three-valued_logic

          Show
          prafulla Prafulla T added a comment - - edited Sure, Here are my review comments. — Float4Datum::compareTo should raise exception when NullValue is seen? Just to be safe? I see that we can do this in almost all other classes Int,Text etc. Type of a > NULL should be Boolean OR NULL ? If NULL, we need to change type determination logic? For NULL with AND /OR, we need to consider three value logic? https://en.wikipedia.org/wiki/Three-valued_logic —
          Hide
          prafulla Prafulla T added a comment -

          I noticed that in patch there is inconsistency of whitespace vs tabs ( I think, you used whitespaces and current code uses tab). I think, we should make it consistent.

          Show
          prafulla Prafulla T added a comment - I noticed that in patch there is inconsistency of whitespace vs tabs ( I think, you used whitespaces and current code uses tab). I think, we should make it consistent.
          Hide
          hyunsik Hyunsik Choi added a comment -

          Float4Datum::compareTo should raise exception when NullValue is seen? Just to be safe?
          I see that we can do this in almost all other classes Int,Text etc.

          Float4Datum::compareTo is returning -1. Others are also the same. This may be caused some inconstant use of brackets.

          Float4Datum::compareTo
            @Override
            public int compareTo(Datum datum) {
              switch (datum.type()) {
                ...      
                ...
                case FLOAT8: {
                  double another = datum.asFloat8();
                  if (val < another) {
                    return -1;
                  } else if (val > another) {
                    return 1;
                  } else {
                    return 0;
                  }
                }
                case NULL_TYPE:
                  return -1;
                default:
                  throw new InvalidOperationException();
              }
            }
          

          Type of a > NULL should be Boolean OR NULL ? If NULL, we need to change type determination logic?

          For NULL with AND /OR, we need to consider three value logic?
          https://en.wikipedia.org/wiki/Three-valued_logic

          Very nice questions! Yes, I've considered three valued logic even for AND and OR cases. I'll also add additional unit tests for AND and OR cases. I think we don't need to change the type determination logic. Type determination logic is only used to determine the expected schema. NULL can be occur from only field data.

          Also, I'll fix inconsistency of whitespace and tabs. In these days, I'm feeling the needs of automatic coding standard verification by using maven checkstyle plugin.

          Thanks, and have nice holidays.

          Show
          hyunsik Hyunsik Choi added a comment - Float4Datum::compareTo should raise exception when NullValue is seen? Just to be safe? I see that we can do this in almost all other classes Int,Text etc. Float4Datum::compareTo is returning -1. Others are also the same. This may be caused some inconstant use of brackets. Float4Datum::compareTo @Override public int compareTo(Datum datum) { switch (datum.type()) { ... ... case FLOAT8: { double another = datum.asFloat8(); if (val < another) { return -1; } else if (val > another) { return 1; } else { return 0; } } case NULL_TYPE: return -1; default : throw new InvalidOperationException(); } } Type of a > NULL should be Boolean OR NULL ? If NULL, we need to change type determination logic? For NULL with AND /OR, we need to consider three value logic? https://en.wikipedia.org/wiki/Three-valued_logic Very nice questions! Yes, I've considered three valued logic even for AND and OR cases. I'll also add additional unit tests for AND and OR cases. I think we don't need to change the type determination logic. Type determination logic is only used to determine the expected schema. NULL can be occur from only field data. Also, I'll fix inconsistency of whitespace and tabs. In these days, I'm feeling the needs of automatic coding standard verification by using maven checkstyle plugin. Thanks, and have nice holidays.
          Hide
          prafulla Prafulla T added a comment -

          Thanks for you replies.
          Let me know if I mis-understood the changes, but I believe following AND would not return correct result in some cases.

                 case AND: {
          -        boolean left = leftExpr.terminate(binCtx.left).asBool();
          -        boolean right = rightExpr.terminate(binCtx.right).asBool();
          -        return DatumFactory.createBool(left && right);
          +        if (lhs.type() == Type.NULL_TYPE || rhs.type() == Type.NULL_TYPE) {
          +          return NullDatum.get();
          +        }
          +        return DatumFactory.createBool(lhs.asBool() && rhs.asBool());
          

          This would return NULL for "false AND NULL" ? But we need it to return false?
          I think we can create similar case for OR as well.

          Show
          prafulla Prafulla T added a comment - Thanks for you replies. Let me know if I mis-understood the changes, but I believe following AND would not return correct result in some cases. case AND: { - boolean left = leftExpr.terminate(binCtx.left).asBool(); - boolean right = rightExpr.terminate(binCtx.right).asBool(); - return DatumFactory.createBool(left && right); + if (lhs.type() == Type.NULL_TYPE || rhs.type() == Type.NULL_TYPE) { + return NullDatum.get(); + } + return DatumFactory.createBool(lhs.asBool() && rhs.asBool()); This would return NULL for "false AND NULL" ? But we need it to return false? I think we can create similar case for OR as well.
          Hide
          hyunsik Hyunsik Choi added a comment -

          You are right. I misunderstood three value logic for AND and OR. I'll correct it! Thanks!

          Show
          hyunsik Hyunsik Choi added a comment - You are right. I misunderstood three value logic for AND and OR. I'll correct it! Thanks!
          Hide
          hyunsik Hyunsik Choi added a comment -

          I've uploaded the second patch. It resolved three valued logic for AND and OR, and other problems caused by the change.

          Show
          hyunsik Hyunsik Choi added a comment - I've uploaded the second patch. It resolved three valued logic for AND and OR, and other problems caused by the change.
          Hide
          prafulla Prafulla T added a comment -

          +1
          Changes related to this ticket look good. I liked how you implemented Three Valued Logic.
          I see that patch has fixes for some other tickets as well but does not contain relevant changes to Change.txt

          Show
          prafulla Prafulla T added a comment - +1 Changes related to this ticket look good. I liked how you implemented Three Valued Logic. I see that patch has fixes for some other tickets as well but does not contain relevant changes to Change.txt
          Hide
          hyunsik Hyunsik Choi added a comment -

          Thank you for the review. I'll add the relevant change log to CHANGES.txt before commit. After this patch get one +1 from one of committers, I'll commit it.

          Show
          hyunsik Hyunsik Choi added a comment - Thank you for the review. I'll add the relevant change log to CHANGES.txt before commit. After this patch get one +1 from one of committers, I'll commit it.
          Hide
          jihoonson Jihoon Son added a comment -

          I'll review the patch.

          Show
          jihoonson Jihoon Son added a comment - I'll review the patch.
          Hide
          jihoonson Jihoon Son added a comment -

          The patch is hard to review, because it includes multiple issues.
          Anyway, in overall, the patch looks good to me.
          I suggest two things as follows.

          • In Inet4Datum.compareTo(), you look to miss when two datums are equal. A code should be added below the for clause as follows
                case INET4:
                  byte [] bytes = asByteArray();
                  byte [] other = datum.asByteArray();
                  
                  for (int i = 0; i < 4; i++) {
                    if (bytes[i] > other[i]) {
                      return 1;
                    } else if (bytes[i] < other[i]) {
                      return -1;
                    }
                  }
                  return 0; // should be added
            
          • How about define integer values to represent three-values? I think that it would increase the readability. Here is an example.
            Current
              public Datum and(Datum datum) {
                return BooleanDatum.AND_LOGIC[0][datum.asInt4()];
              }
            

            Suggestion

              public Datum and(Datum datum) {
                return BooleanDatum.AND_LOGIC[UNKNOWN_INTEGER][datum.asInt4()];
              }
            
          Show
          jihoonson Jihoon Son added a comment - The patch is hard to review, because it includes multiple issues. Anyway, in overall, the patch looks good to me. I suggest two things as follows. In Inet4Datum.compareTo(), you look to miss when two datums are equal. A code should be added below the for clause as follows case INET4: byte [] bytes = asByteArray(); byte [] other = datum.asByteArray(); for ( int i = 0; i < 4; i++) { if (bytes[i] > other[i]) { return 1; } else if (bytes[i] < other[i]) { return -1; } } return 0; // should be added How about define integer values to represent three-values? I think that it would increase the readability. Here is an example. Current public Datum and(Datum datum) { return BooleanDatum.AND_LOGIC[0][datum.asInt4()]; } Suggestion public Datum and(Datum datum) { return BooleanDatum.AND_LOGIC[UNKNOWN_INTEGER][datum.asInt4()]; }
          Hide
          hyunsik Hyunsik Choi added a comment -

          Thank you Jihoon for your review!

          Yes, this patch includes many issues, but almost of all modifications are for NULL value handling and bug fixes caused by the correction of null value handling. For example, after I corrected null comparison, scan filter and outer join operators incurred errors. This patch also includes the fix.

          Actually, so far, we have considered a little bit of null values handling in many parts of datums and expressions. In this issue, I've tried to cover all null value comparisons throughout the code. In order to represent the goal of this issue, I've changed the issue title to 'Correct Null value handling of primitive operators.

          I'm still not sure that it patch covers all parts. I think that we should continue to find and fix the wrong code that we haven't found. In addition, we should add more unit tests.

          I've uploaded the third patch. This patch reflects your suggestion and includes the fix of missed null value handling.

          Merry Christmas!

          Show
          hyunsik Hyunsik Choi added a comment - Thank you Jihoon for your review! Yes, this patch includes many issues, but almost of all modifications are for NULL value handling and bug fixes caused by the correction of null value handling. For example, after I corrected null comparison, scan filter and outer join operators incurred errors. This patch also includes the fix. Actually, so far, we have considered a little bit of null values handling in many parts of datums and expressions. In this issue, I've tried to cover all null value comparisons throughout the code. In order to represent the goal of this issue, I've changed the issue title to 'Correct Null value handling of primitive operators. I'm still not sure that it patch covers all parts. I think that we should continue to find and fix the wrong code that we haven't found. In addition, we should add more unit tests. I've uploaded the third patch. This patch reflects your suggestion and includes the fix of missed null value handling. Merry Christmas!
          Hide
          jhkim Jinho Kim added a comment -

          +1
          This patch looks great to me.

          Show
          jhkim Jinho Kim added a comment - +1 This patch looks great to me.
          Hide
          hyunsik Hyunsik Choi added a comment -

          committed. Thank you guys for the reviews.

          Show
          hyunsik Hyunsik Choi added a comment - committed. Thank you guys for the reviews.
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Tajo-trunk-postcommit #634 (See https://builds.apache.org/job/Tajo-trunk-postcommit/634/)
          TAJO-182: Correct NULL value handling of primitive operators. (hyunsik) (hyunsik: https://git-wip-us.apache.org/repos/asf?p=incubator-tajo.git&a=commit&h=e8c5c27717e134264a93a0430521a07c45079215)

          • tajo-common/src/main/java/org/apache/tajo/datum/Inet4Datum.java
          • tajo-common/src/main/java/org/apache/tajo/datum/BitDatum.java
          • tajo-common/src/main/java/org/apache/tajo/datum/TimeDatum.java
          • tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/eval/PatternMatchPredicateEval.java
          • tajo-core/tajo-core-backend/src/test/java/org/apache/tajo/engine/eval/TestSQLExpression.java
          • tajo-common/src/main/java/org/apache/tajo/datum/Int8Datum.java
          • tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/physical/NLLeftOuterJoinExec.java
          • tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/parser/SQLAnalyzer.java
          • tajo-common/src/main/java/org/apache/tajo/datum/CharDatum.java
          • CHANGES.txt
          • tajo-core/tajo-core-backend/src/test/java/org/apache/tajo/engine/query/TestJoinQuery.java
          • tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/physical/HashLeftOuterJoinExec.java
          • tajo-common/src/main/java/org/apache/tajo/datum/TimestampDatum.java
          • tajo-common/src/main/java/org/apache/tajo/datum/DateDatum.java
          • tajo-common/src/main/java/org/apache/tajo/datum/Int4Datum.java
          • tajo-core/tajo-core-backend/src/test/java/org/apache/tajo/engine/function/TestStringOperatorsAndFunctions.java
          • tajo-common/src/main/java/org/apache/tajo/datum/Int2Datum.java
          • tajo-common/src/main/java/org/apache/tajo/datum/NullDatum.java
          • tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/eval/BinaryEval.java
          • tajo-common/src/main/java/org/apache/tajo/datum/Float4Datum.java
          • tajo-common/src/main/java/org/apache/tajo/datum/BooleanDatum.java
          • tajo-common/src/main/java/org/apache/tajo/datum/BlobDatum.java
          • tajo-core/tajo-core-backend/src/main/antlr4/org/apache/tajo/engine/parser/SQLParser.g4
          • tajo-common/src/main/java/org/apache/tajo/datum/DatumFactory.java
          • tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/physical/HashFullOuterJoinExec.java
          • tajo-common/src/test/java/org/apache/tajo/datum/TestBoolDatum.java
          • tajo-common/src/main/java/org/apache/tajo/datum/Float8Datum.java
          • tajo-common/src/main/java/org/apache/tajo/datum/TextDatum.java
          • tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/eval/CastEval.java
          • tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/physical/SeqScanExec.java
          • tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/function/InCountry.java
          • tajo-common/src/main/java/org/apache/tajo/datum/Datum.java
          • tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/eval/InEval.java
          • tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/function/string/SplitPart.java
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Tajo-trunk-postcommit #634 (See https://builds.apache.org/job/Tajo-trunk-postcommit/634/ ) TAJO-182 : Correct NULL value handling of primitive operators. (hyunsik) (hyunsik: https://git-wip-us.apache.org/repos/asf?p=incubator-tajo.git&a=commit&h=e8c5c27717e134264a93a0430521a07c45079215 ) tajo-common/src/main/java/org/apache/tajo/datum/Inet4Datum.java tajo-common/src/main/java/org/apache/tajo/datum/BitDatum.java tajo-common/src/main/java/org/apache/tajo/datum/TimeDatum.java tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/eval/PatternMatchPredicateEval.java tajo-core/tajo-core-backend/src/test/java/org/apache/tajo/engine/eval/TestSQLExpression.java tajo-common/src/main/java/org/apache/tajo/datum/Int8Datum.java tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/physical/NLLeftOuterJoinExec.java tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/parser/SQLAnalyzer.java tajo-common/src/main/java/org/apache/tajo/datum/CharDatum.java CHANGES.txt tajo-core/tajo-core-backend/src/test/java/org/apache/tajo/engine/query/TestJoinQuery.java tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/physical/HashLeftOuterJoinExec.java tajo-common/src/main/java/org/apache/tajo/datum/TimestampDatum.java tajo-common/src/main/java/org/apache/tajo/datum/DateDatum.java tajo-common/src/main/java/org/apache/tajo/datum/Int4Datum.java tajo-core/tajo-core-backend/src/test/java/org/apache/tajo/engine/function/TestStringOperatorsAndFunctions.java tajo-common/src/main/java/org/apache/tajo/datum/Int2Datum.java tajo-common/src/main/java/org/apache/tajo/datum/NullDatum.java tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/eval/BinaryEval.java tajo-common/src/main/java/org/apache/tajo/datum/Float4Datum.java tajo-common/src/main/java/org/apache/tajo/datum/BooleanDatum.java tajo-common/src/main/java/org/apache/tajo/datum/BlobDatum.java tajo-core/tajo-core-backend/src/main/antlr4/org/apache/tajo/engine/parser/SQLParser.g4 tajo-common/src/main/java/org/apache/tajo/datum/DatumFactory.java tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/physical/HashFullOuterJoinExec.java tajo-common/src/test/java/org/apache/tajo/datum/TestBoolDatum.java tajo-common/src/main/java/org/apache/tajo/datum/Float8Datum.java tajo-common/src/main/java/org/apache/tajo/datum/TextDatum.java tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/eval/CastEval.java tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/physical/SeqScanExec.java tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/function/InCountry.java tajo-common/src/main/java/org/apache/tajo/datum/Datum.java tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/eval/InEval.java tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/function/string/SplitPart.java
          Hide
          jihoonson Jihoon Son added a comment -

          Although this patch is already committed, I have some suggestions to improve the patch.

          • In Inet4Datum.compareTo(), we can sure that two datums are equal when all the bytes are same. Thus, the code should be changed as follows.
              public int compareTo(Datum datum) {
                switch (datum.type()) {
                case INET4:
                  byte [] bytes = asByteArray();
                  byte [] other = datum.asByteArray();
                  
                  for (int i = 0; i < 4; i++) {
                    if (bytes[i] > other[i]) {
                      return 1;
                    } else if (bytes[i] < other[i]) {
                      return -1;
                    }
                  }
                  return 0; // should be placed after the for clause
                  ...
              }
            
          • In TestBoolDatum.testAsBool(), the first line looks to be intended to test the true datum, but it contains the false. It would be better to change the code as follows.
            Datum trueDatum = DatumFactory.createBool(true); // create a boolean datum with true instead of false
            
          Show
          jihoonson Jihoon Son added a comment - Although this patch is already committed, I have some suggestions to improve the patch. In Inet4Datum.compareTo(), we can sure that two datums are equal when all the bytes are same. Thus, the code should be changed as follows. public int compareTo(Datum datum) { switch (datum.type()) { case INET4: byte [] bytes = asByteArray(); byte [] other = datum.asByteArray(); for ( int i = 0; i < 4; i++) { if (bytes[i] > other[i]) { return 1; } else if (bytes[i] < other[i]) { return -1; } } return 0; // should be placed after the for clause ... } In TestBoolDatum.testAsBool(), the first line looks to be intended to test the true datum, but it contains the false. It would be better to change the code as follows. Datum trueDatum = DatumFactory.createBool( true ); // create a boolean datum with true instead of false
          Hide
          jhkim Jinho Kim added a comment -

          Jihoon,
          Sorry, I missed them.
          I will create new jira issue.
          Thanks.

          Show
          jhkim Jinho Kim added a comment - Jihoon, Sorry, I missed them. I will create new jira issue. Thanks.
          Hide
          jihoonson Jihoon Son added a comment -

          Thanks, Jinho!

          Show
          jihoonson Jihoon Son added a comment - Thanks, Jinho!

            People

            • Assignee:
              hyunsik Hyunsik Choi
              Reporter:
              hyunsik Hyunsik Choi
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development