From 13a2dc54adf27aeb32406e0c9e4662c46e81e5ee Mon Sep 17 00:00:00 2001
From: gmantele <gmantele@ari.uni-heidelberg.de>
Date: Thu, 27 Aug 2015 14:39:40 +0200
Subject: [PATCH] [ADQL] Fix a Big Bug reported by M.Taylor and M.Demleitner:
in ORDER BY, GROUP BY and USING only regular and delimited identifiers are
accepted, not qualified column names. For instance: "SELECT table.column_name
FROM table ORDER BY table.column_name" is wrong. We should instead write:
"SELECT table.column_name FROM table ORDER BY column_name". "SELECT
table.column_name AS mycol FROM table ORDER BY mycol" is also correct. Of
course, for ORDER BY and GROUP BY, it is still possible to reference a column
using its index in the SELECT clause. For instance: "SELECT table.column_name
FROM table ORDER BY 1".
---
src/adql/parser/ADQLParser.java | 20 ++++----
src/adql/parser/ADQLQueryFactory.java | 26 +++++++++-
src/adql/parser/adqlGrammar.jj | 22 ++++-----
test/adql/parser/TestADQLParser.java | 69 ++++++++++++++++++++++++++-
4 files changed, 114 insertions(+), 23 deletions(-)
diff --git a/src/adql/parser/ADQLParser.java b/src/adql/parser/ADQLParser.java
index 80c3990..c98e3d9 100644
--- a/src/adql/parser/ADQLParser.java
+++ b/src/adql/parser/ADQLParser.java
@@ -67,7 +67,7 @@ import adql.translator.TranslationException;
* @see ADQLQueryFactory
*
* @author Grégory Mantelet (CDS;ARI) - gmantele@ari.uni-heidelberg.de
-* @version 1.4 (06/2015)
+* @version 1.4 (08/2015)
*/
public class ADQLParser implements ADQLParserConstants {
@@ -1074,12 +1074,12 @@ public class ADQLParser implements ADQLParserConstants {
final public ColumnReference ColumnRef() throws ParseException{
trace_call("ColumnRef");
try{
- IdentifierItems identifiers = null;
+ IdentifierItem identifier = null;
Token ind = null;
switch((jj_ntk == -1) ? jj_ntk() : jj_ntk){
case DELIMITED_IDENTIFIER:
case REGULAR_IDENTIFIER:
- identifiers = ColumnName();
+ identifier = Identifier();
break;
case UNSIGNED_INTEGER:
ind = jj_consume_token(UNSIGNED_INTEGER);
@@ -1091,8 +1091,8 @@ public class ADQLParser implements ADQLParserConstants {
}
try{
ColumnReference colRef = null;
- if (identifiers != null)
- colRef = queryFactory.createColRef(identifiers);
+ if (identifier != null)
+ colRef = queryFactory.createColRef(identifier);
else
colRef = queryFactory.createColRef(Integer.parseInt(ind.image), new TextPosition(ind));
{
@@ -1114,12 +1114,12 @@ public class ADQLParser implements ADQLParserConstants {
final public ADQLOrder OrderItem() throws ParseException{
trace_call("OrderItem");
try{
- IdentifierItems identifiers = null;
+ IdentifierItem identifier = null;
Token ind = null, desc = null;
switch((jj_ntk == -1) ? jj_ntk() : jj_ntk){
case DELIMITED_IDENTIFIER:
case REGULAR_IDENTIFIER:
- identifiers = ColumnName();
+ identifier = Identifier();
break;
case UNSIGNED_INTEGER:
ind = jj_consume_token(UNSIGNED_INTEGER);
@@ -1151,9 +1151,9 @@ public class ADQLParser implements ADQLParserConstants {
}
try{
ADQLOrder order = null;
- if (identifiers != null){
- order = queryFactory.createOrder(identifiers, desc != null);
- order.setPosition(identifiers.getPosition());
+ if (identifier != null){
+ order = queryFactory.createOrder(identifier, desc != null);
+ order.setPosition(identifier.position);
}else{
order = queryFactory.createOrder(Integer.parseInt(ind.image), desc != null);
order.setPosition(new TextPosition(ind));
diff --git a/src/adql/parser/ADQLQueryFactory.java b/src/adql/parser/ADQLQueryFactory.java
index 4bcfdf2..e10931e 100644
--- a/src/adql/parser/ADQLQueryFactory.java
+++ b/src/adql/parser/ADQLQueryFactory.java
@@ -83,7 +83,7 @@ import adql.query.operand.function.geometry.RegionFunction;
* <p>To customize the object representation you merely have to extends the appropriate functions of this class.</p>
*
* @author Grégory Mantelet (CDS;ARI)
- * @version 1.4 (06/2015)
+ * @version 1.4 (08/2015)
*
* @see ADQLParser
*/
@@ -403,6 +403,17 @@ public class ADQLQueryFactory {
return order;
}
+ public ADQLOrder createOrder(final IdentifierItem colName, final boolean desc) throws Exception{
+ ADQLOrder order = new ADQLOrder(colName.identifier, desc);
+ if (order != null)
+ order.setCaseSensitive(colName.caseSensitivity);
+ return order;
+ }
+
+ /**
+ * @deprecated since 1.4 ; Former version's mistake: an ORDER BY item is either a regular/delimited column name or an integer, not a qualified column name ; Replaced by {@link #createOrder(Identifier, boolean)} ; This function is no longer used by ADQLParser.
+ */
+ @Deprecated
public ADQLOrder createOrder(final IdentifierItems idItems, final boolean desc) throws Exception{
ADQLOrder order = new ADQLOrder(idItems.join("."), desc);
if (order != null)
@@ -410,6 +421,19 @@ public class ADQLQueryFactory {
return order;
}
+ public ColumnReference createColRef(final IdentifierItem idItem) throws Exception{
+ ColumnReference colRef = new ColumnReference(idItem.identifier);
+ if (colRef != null){
+ colRef.setPosition(idItem.position);
+ colRef.setCaseSensitive(idItem.caseSensitivity);
+ }
+ return colRef;
+ }
+
+ /**
+ * @deprecated since 1.4 ; Former version's mistake: a GROUP BY item is either a regular/delimited column name or an integer, not a qualified column name ; Replaced by {@link #createColRef(Identifier)} ; This function is no longer used by ADQLParser.
+ */
+ @Deprecated
public ColumnReference createColRef(final IdentifierItems idItems) throws Exception{
ColumnReference colRef = new ColumnReference(idItems.join("."));
if (colRef != null){
diff --git a/src/adql/parser/adqlGrammar.jj b/src/adql/parser/adqlGrammar.jj
index 2b771a9..7d4c062 100644
--- a/src/adql/parser/adqlGrammar.jj
+++ b/src/adql/parser/adqlGrammar.jj
@@ -26,7 +26,7 @@
* If the syntax is not conform to the ADQL definition an error message is printed else it will be the message "Correct syntax".
*
* Author: Grégory Mantelet (CDS;ARI) - gmantele@ari.uni-heidelberg.de
-* Version: 1.4 (06/2015)
+* Version: 1.4 (08/2015)
*/
/* ########### */
@@ -89,7 +89,7 @@ import adql.translator.TranslationException;
* @see ADQLQueryFactory
*
* @author Grégory Mantelet (CDS;ARI) - gmantele@ari.uni-heidelberg.de
-* @version 1.4 (06/2015)
+* @version 1.4 (08/2015)
*/
public class ADQLParser {
@@ -918,13 +918,13 @@ ADQLColumn Column(): {IdentifierItems identifiers;} {
}
}
-ColumnReference ColumnRef(): {IdentifierItems identifiers = null; Token ind = null;}{
- ( identifiers=ColumnName() | ind=<UNSIGNED_INTEGER> )
+ColumnReference ColumnRef(): {IdentifierItem identifier = null; Token ind = null;}{
+ ( identifier=Identifier() | ind=<UNSIGNED_INTEGER> )
{
try{
ColumnReference colRef = null;
- if (identifiers != null)
- colRef = queryFactory.createColRef(identifiers);
+ if (identifier != null)
+ colRef = queryFactory.createColRef(identifier);
else
colRef = queryFactory.createColRef(Integer.parseInt(ind.image), new TextPosition(ind));
return colRef;
@@ -934,14 +934,14 @@ ColumnReference ColumnRef(): {IdentifierItems identifiers = null; Token ind = nu
}
}
-ADQLOrder OrderItem(): {IdentifierItems identifiers = null; Token ind = null, desc = null;}{
- (identifiers=ColumnName() | ind=<UNSIGNED_INTEGER>) (<ASC> | desc=<DESC>)?
+ADQLOrder OrderItem(): {IdentifierItem identifier = null; Token ind = null, desc = null;}{
+ (identifier=Identifier() | ind=<UNSIGNED_INTEGER>) (<ASC> | desc=<DESC>)?
{
try{
ADQLOrder order = null;
- if (identifiers != null){
- order = queryFactory.createOrder(identifiers, desc!=null);
- order.setPosition(identifiers.getPosition());
+ if (identifier != null){
+ order = queryFactory.createOrder(identifier, desc!=null);
+ order.setPosition(identifier.position);
}else{
order = queryFactory.createOrder(Integer.parseInt(ind.image), desc!=null);
order.setPosition(new TextPosition(ind));
diff --git a/test/adql/parser/TestADQLParser.java b/test/adql/parser/TestADQLParser.java
index 82366d6..1272d67 100644
--- a/test/adql/parser/TestADQLParser.java
+++ b/test/adql/parser/TestADQLParser.java
@@ -26,7 +26,74 @@ public class TestADQLParser {
@After
public void tearDown() throws Exception{}
-
+
+ @Test
+ public void testColumnReference(){
+ ADQLParser parser = new ADQLParser();
+ try{
+ // ORDER BY
+ parser.parseQuery("SELECT * FROM cat ORDER BY oid;");
+ parser.parseQuery("SELECT * FROM cat ORDER BY oid ASC;");
+ parser.parseQuery("SELECT * FROM cat ORDER BY oid DESC;");
+ parser.parseQuery("SELECT * FROM cat ORDER BY 1;");
+ parser.parseQuery("SELECT * FROM cat ORDER BY 1 ASC;");
+ parser.parseQuery("SELECT * FROM cat ORDER BY 1 DESC;");
+ // GROUP BY
+ parser.parseQuery("SELECT * FROM cat GROUP BY oid;");
+ parser.parseQuery("SELECT * FROM cat GROUP BY 1;");
+ // JOIN ... USING(...)
+ parser.parseQuery("SELECT * FROM cat JOIN cat2 USING(oid);");
+ }catch(Exception e){
+ e.printStackTrace(System.err);
+ fail("These ADQL queries are strictly correct! No error should have occured. (see stdout for more details)");
+ }
+
+ try{
+ // ORDER BY
+ parser.parseQuery("SELECT * FROM cat ORDER BY cat.oid;");
+ fail("A qualified column name is forbidden in ORDER BY! This test should have failed.");
+ }catch(Exception e){
+ assertEquals(ParseException.class, e.getClass());
+ assertEquals(" Encountered \".\". Was expecting one of: <EOF> \",\" \";\" \"ASC\" \"DESC\" ", e.getMessage());
+ }
+
+ // Query reported as in error before the bug correction:
+ try{
+ parser.parseQuery("SELECT TOP 10 browndwarfs.cat.jmag FROM browndwarfs.cat ORDER BY browndwarfs.cat.jmag");
+ fail("A qualified column name is forbidden in ORDER BY! This test should have failed.");
+ }catch(Exception e){
+ assertEquals(ParseException.class, e.getClass());
+ assertEquals(" Encountered \".\". Was expecting one of: <EOF> \",\" \";\" \"ASC\" \"DESC\" ", e.getMessage());
+ }
+
+ try{
+ // GROUP BY
+ parser.parseQuery("SELECT * FROM cat GROUP BY cat.oid;");
+ fail("A qualified column name is forbidden in ORDER BY! This test should have failed.");
+ }catch(Exception e){
+ assertEquals(ParseException.class, e.getClass());
+ assertEquals(" Encountered \".\". Was expecting one of: <EOF> \",\" \";\" \"HAVING\" \"ORDER BY\" ", e.getMessage());
+ }
+
+ try{
+ // JOIN ... USING(...)
+ parser.parseQuery("SELECT * FROM cat JOIN cat2 USING(cat.oid);");
+ fail("A qualified column name is forbidden in USING(...)! This test should have failed.");
+ }catch(Exception e){
+ assertEquals(ParseException.class, e.getClass());
+ assertEquals(" Encountered \".\". Was expecting one of: \")\" \",\" ", e.getMessage());
+ }
+
+ try{
+ // JOIN ... USING(...)
+ parser.parseQuery("SELECT * FROM cat JOIN cat2 USING(1);");
+ fail("A column index is forbidden in USING(...)! This test should have failed.");
+ }catch(Exception e){
+ assertEquals(ParseException.class, e.getClass());
+ assertEquals(" Encountered \"1\". Was expecting one of: \"\\\"\" <REGULAR_IDENTIFIER> ", e.getMessage());
+ }
+ }
+
@Test
public void testDelimitedIdentifiersWithDot(){
ADQLParser parser = new ADQLParser();
--
GitLab