Review Board 1.7.22


NOT expression doesn't handle nulls correctly.

Review Request #14576 - Created Oct. 10, 2013 and updated

Jitendra Pandey
trunk
HIVE-5430
Reviewers
hive
ashutoshc, hanson5b
hive-git
NOT expression doesn't handle nulls correctly.

 
Total:
24
Open:
24
Resolved:
0
Dropped:
0
Status:
From:
Description From Last Updated Status
This is a minor point, but I think setNumArguments is redundant. It can be inferred fromthe number of arguments passed ... Eric Hanson Oct. 23, 2013, 12:03 a.m. Open
Nice job encapsulating this in a class. That is better than exposing the bitmap externally. Eric Hanson Oct. 23, 2013, 12:03 a.m. Open
Should say "number of" not "number if". Why assume max 3 arguments? Is it necessary to have a max? Please ... Eric Hanson Oct. 23, 2013, 12:03 a.m. Open
Nice. Eric Hanson Oct. 23, 2013, 12:03 a.m. Open
Please add a comment to say what this does and how it works. Consider making this method static. Eric Hanson Oct. 23, 2013, 12:03 a.m. Open
Please add a comment to describe vMap Also, please add private keyword if it is private Eric Hanson Oct. 23, 2013, 12:03 a.m. Open
Can you elaborate more here in the comments? I'm not sure int what situations filter mode is used here and ... Eric Hanson Oct. 23, 2013, 12:03 a.m. Open
extra blan space after VectorExpression Eric Hanson Oct. 23, 2013, 12:03 a.m. Open
add blank line before comments Eric Hanson Oct. 23, 2013, 12:03 a.m. Open
Please put a comment to explain this line (you are putting the outputCol always as the last arg...) Eric Hanson Oct. 23, 2013, 12:03 a.m. Open
Please add a comment to describe the purpose of this interface. Eric Hanson Oct. 23, 2013, 12:03 a.m. Open
Please add a comment to describe the purpose of this class. Eric Hanson Oct. 23, 2013, 12:03 a.m. Open
please add a comment to explain the logic of this evaluate() method Eric Hanson Oct. 23, 2013, 12:03 a.m. Open
please add comment to say the purpose of this class Eric Hanson Oct. 23, 2013, 12:03 a.m. Open
It'd be good to format this like the other getDescriptor methods are formatted, for consistency Eric Hanson Oct. 23, 2013, 12:03 a.m. Open
is it the case that this method and the ISetDoubleArg interface are not needed for this class any more? Eric Hanson Oct. 23, 2013, 12:03 a.m. Open
Is this interface needed any more? Eric Hanson Oct. 23, 2013, 12:03 a.m. Open
Is ISetLongArg need any more? Eric Hanson Oct. 23, 2013, 12:03 a.m. Open
is iSetLongArg needed here any more? Eric Hanson Oct. 23, 2013, 12:03 a.m. Open
Did you do an end-to-end test with the UDF Adaptor to make sure it still works? The unit test should ... Eric Hanson Oct. 23, 2013, 12:03 a.m. Open
I think this .setArg call is now redundant given the constructor change. Eric Hanson Oct. 23, 2013, 12:03 a.m. Open
remove commented line? Eric Hanson Oct. 23, 2013, 12:03 a.m. Open
remove commented line? Eric Hanson Oct. 23, 2013, 12:03 a.m. Open
All the getBytes() calls should be getBytes("UTF-8"). It may actually matter for the multiByte char cases. Eric Hanson Oct. 23, 2013, 12:03 a.m. Open
Review request changed
Updated (Oct. 21, 2013, 6 p.m.)
Posted (Oct. 23, 2013, 12:03 a.m.)

   

  
This is a minor point, but I think setNumArguments is redundant. It can be inferred fromthe number of arguments passed to setArgumentTypes and setInputExpressionTypes.
Nice job encapsulating this in a class. That is better than exposing the bitmap externally.
Should say "number of" not "number if".

Why assume max 3 arguments? Is it necessary to have a max? Please explain in the comment.
Nice.
Please add a comment to say what this does and how it works.

Consider making this method static.
Please add a comment to describe vMap

Also, please add private keyword if it is private
Can you elaborate more here in the comments? I'm not sure int what situations filter mode is used here and why you add the SelectColumnIsTrue. Is it for NOT?

Also, the coding standards as for a blank line before all comment lines.
extra blan space after VectorExpression
add blank line before comments
Please put a comment to explain this line (you are putting the outputCol always as the last arg...)
Please add a comment to describe the purpose of this interface.
Please add a comment to describe the purpose of this class.
please add a comment to explain the logic of this evaluate() method
please add comment to say the purpose of this class
It'd be good to format this like the other getDescriptor methods are formatted, for consistency
is it the case that this method and the ISetDoubleArg interface are not needed for this class any more?
Is this interface needed any more?
Is ISetLongArg need any more?
is iSetLongArg needed here any more?
Did you do an end-to-end test with the UDF Adaptor to make sure it still works?

The unit test should cover it though.
I think this .setArg call is now redundant given the constructor change.
remove commented line?
remove commented line?
All the getBytes() calls should be getBytes("UTF-8").

It may actually matter for the multiByte char cases.