NullPointerException with NULLIF in GROUP BY clause

• Key: DERBY-2014
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Major
• Assignee: Yip Ng
• Reporter: Yip Ng
• Component/s: SQL
• Affects Version/s: 10.2.1.6, 10.3.1.4
• Fix Version/s: 10.2.2.0, 10.3.1.4

Description

A NPE occurs when NULLIF is used in the GROUP BY clause.

ij> create table t1 (c1 int);
0 rows inserted/updated/deleted
ij> insert into t1 values 1,2,2,3;
4 rows inserted/updated/deleted
ij> select nullif(c1,c1) from t1 group by nullif(c1,c1);
ERROR XJ001: Java exception: ': java.lang.NullPointerException'.

Stacktrace:

java.lang.NullPointerException
at org.apache.derby.impl.sql.compile.ConstantNode.isEquivalent(ConstantNode.java:285)
at org.apache.derby.impl.sql.compile.CastNode.isEquivalent(CastNode.java:1044)
at org.apache.derby.impl.sql.compile.ConditionalNode.isEquivalent(ConditionalNode.java:518)
at org.apache.derby.impl.sql.compile.GroupByList.findGroupingColumn(GroupByList.java:244)
at org.apache.derby.impl.sql.compile.VerifyAggregateExpressionsVisitor.skipChildren(VerifyAggregateExpressionsVisitor.java:146)
at org.apache.derby.impl.sql.compile.ConditionalNode.accept(ConditionalNode.java:484)
at org.apache.derby.impl.sql.compile.ResultColumn.accept(ResultColumn.java:1515)
at org.apache.derby.impl.sql.compile.QueryTreeNodeVector.accept(QueryTreeNodeVector.java:159)
at org.apache.derby.impl.sql.compile.SelectNode.bindExpressions(SelectNode.java:619)
at org.apache.derby.impl.sql.compile.FromSubquery.bindExpressions(FromSubquery.java:262)
at org.apache.derby.impl.sql.compile.FromList.bindExpressions(FromList.java:337)
at org.apache.derby.impl.sql.compile.SelectNode.bindExpressions(SelectNode.java:500)
at org.apache.derby.impl.sql.compile.DMLStatementNode.bindExpressions(DMLStatementNode.java:249)
at org.apache.derby.impl.sql.compile.DMLStatementNode.bind(DMLStatementNode.java:162)
at org.apache.derby.impl.sql.compile.CursorNode.bind(CursorNode.java:253)
at org.apache.derby.impl.sql.GenericStatement.prepMinion(GenericStatement.java:345)
at org.apache.derby.impl.sql.GenericStatement.prepare(GenericStatement.java:119)
at org.apache.derby.impl.sql.conn.GenericLanguageConnectionContext.prepareInternalStatement(GenericLanguageConnectionContext.java:745)
at org.apache.derby.impl.jdbc.EmbedStatement.execute(EmbedStatement.java:568)
at org.apache.derby.impl.jdbc.EmbedStatement.execute(EmbedStatement.java:517)
at org.apache.derby.impl.tools.ij.ij.executeImmediate(ij.java:321)
at org.apache.derby.impl.tools.ij.utilMain.doCatch(utilMain.java:517)
at org.apache.derby.impl.tools.ij.utilMain.runScriptGuts(utilMain.java:370)
at org.apache.derby.impl.tools.ij.utilMain.go(utilMain.java:268)
at org.apache.derby.impl.tools.ij.Main.go(Main.java:204)
at org.apache.derby.impl.tools.ij.Main.mainCore(Main.java:170)
at org.apache.derby.impl.tools.ij.Main14.main(Main14.java:56)
at org.apache.derby.tools.ij.main(ij.java:71)

sysinfo:

------------------ Java Information ------------------
Java Version: 1.4.2_12
Java Vendor: Sun Microsystems Inc.
Java home: C:\jdk142\jre
Java classpath: classes;.
OS name: Windows XP
OS architecture: x86
OS version: 5.1
Java user name: yip
Java user home: C:\Documents and Settings\Administrator
Java user dir: C:\derby\trunk
java.specification.name: Java Platform API Specification
java.specification.version: 1.4
--------- Derby Information --------
JRE - JDBC: J2SE 1.4.2 - JDBC 3.0
[C:\derby\trunk\classes] 10.3.0.0 alpha - (1)
------------------------------------------------------
----------------- Locale Information -----------------
Current Locale : [English/United States [en_US]]
Found support for locale: [de_DE]
         version: 10.3.0.0 alpha - (1)
Found support for locale: [es]
         version: 10.3.0.0 alpha - (1)
Found support for locale: [fr]
         version: 10.3.0.0 alpha - (1)
Found support for locale: [it]
         version: 10.3.0.0 alpha - (1)
Found support for locale: [ja_JP]
         version: 10.3.0.0 alpha - (1)
Found support for locale: [ko_KR]
         version: 10.3.0.0 alpha - (1)
Found support for locale: [pt_BR]
         version: 10.3.0.0 alpha - (1)
Found support for locale: [zh_CN]
         version: 10.3.0.0 alpha - (1)
Found support for locale: [zh_TW]
         version: 10.3.0.0 alpha - (1)
------------------------------------------------------

Comments

[Yip Ng]

Attaching patch derby2014-trunk-diff01.txt for DERBY-2014. Again the NPE happens in isEquivalent() method where it does not handle value is null. (same symptom as DERBY-2008) and the patch addresses this + additonal testcases. derbyall + junit suite passes.

[Manish Khettry]

The change to ConstantNode is fine. Thanks for finding and fixing this. On adding tests for this bug: Would it be better to add it to the JUnit test (GroupByExpression.java) rather than the old canon style test?

[Bernt M. Johnsen]

I'm not sure what kind of equivalence isEquivalent, but is the following correct?

+ // value can be null which represents a SQL NULL value.
+ return ( (other.getValue() == null && getValue() == null) ||

I'm always ware wrt. comparision of SQL NULL values, since
    NULL = whatever
(and any other comparision operator) will evaluate to UNKNOWN, not TRUE.

[Yip Ng]

Thanks Manish and Bernt for reviewing the patch.

Bernt: Good question. If I am not mistaken, the isEquivalent() method is used to compare the select column against the group by with expression. Note that it is comparing the structural form of the two expressions for equivalence at bind phase and not comparing the actual row values at runtime to produce a result. Thus, I believe in this context, it is valid.

Manish: I'll see if I can find some time to convert the testcases in the patch to JUnit testcases. Thanks for the suggestion.

[Daniel John Debrunner]

The behaviour of isEquivalent() is described by the javadoc comments for ValueNode.isEquivalent().

Bernt, do you think the description needs to be improved in some way? It's vital to get the javadoc for such a method to be accurate and clear.

Dan is this the Javadoc you are referring to?
http://db.apache.org/derby/javadoc/engine/org/apache/derby/impl/sql/compile/ValueNode.html#isEquivalent(org.apache.derby.impl.sql.compile.ValueNode)

If so, then yes, I think the description needs to be improved. I think
that the topics that need to be described include:
 - equivalency of basic data types, including "interesting" data types
   such as floating point and date types
 - behavior of NULL in equivalency checks
 - equivalency of expressions

Yip, do you think you can propose some candidate Javadoc for us to review?

[Yip Ng]

Sure, I'll be submitting a follow-up patch - addressing junit testcase conversion + the javadoc issue in ValueNode.isEquivalent.

[Yip Ng]

Attaching patch derby2014-trunk-diff02.txt for DERBY-2014. This patch converts all the tests in the previous patch into junit. Also the javadoc for ValueNode.isEquivalent() method has been updated. Please review. (I hope the javadoc is clear, if not, please let me know.)

[Bryan Pendleton]

Thanks Yip! The new Javadoc is clear and detailed and very useful.

The new JUnit tests are also very clear and easy to follow. The only little nit I have is that
they seem to mix tabs and spaces.

[Yip Ng]

Sorry about the mix of tabs and space, I was using 2 different editors. Submitting patch derby2014-trunk-diff03.txt. This patch fixes the mixed tab/spaces and uses 80 character column margin for the updated javadoc for ValueNode.isEquivalent() method. Please review.

[Bryan Pendleton]

I'm ready to commit this, but I wanted to confirm the Javadoc change and am having trouble running 'ant derbydocs', even on a clean trunk. I sent mail to the developer list asking for help.

[Bryan Pendleton]

Andrew fixed the problem I was having with the derbydocs build (thanks Andrew!)

I added a few paragraph and list tags to the ValueNode.isEquivalent javadoc to
make it a bit more friendly in the HTML display and committed the 03 patch to
subversion as revision 471459.

[Rick Hillegas]

Ported to 10.2 branch at subversion revision 480306.

[Andrew McIntyre]

This issue has been resolved for over a year with no further movement. Closing.