Commit 13a2dc54 authored by gmantele's avatar gmantele
Browse files

[ADQL] Fix a Big Bug reported by M.Taylor and M.Demleitner: in ORDER BY, GROUP... 

[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".
parent 84ede4bf

src/adql/parser/ADQLParser.java

+10 −10
Original line number Diff line number Diff line
@@ -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));

src/adql/parser/ADQLQueryFactory.java

+25 −1
Original line number Diff line number Diff line
@@ -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&eacute;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){

src/adql/parser/adqlGrammar.jj

+11 −11
Original line number Diff line number Diff line
@@ -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&eacute;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&eacute;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));

test/adql/parser/TestADQLParser.java

+68 −1
Original line number Diff line number Diff line
@@ -27,6 +27,73 @@ 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();