From 8e2fa9ffc78e92d9b40caaeb97e55f1e7f931e26 Mon Sep 17 00:00:00 2001
From: gmantele <gmantele@ari.uni-heidelberg.de>
Date: Tue, 4 Apr 2017 13:17:30 +0200
Subject: [PATCH] [ADQL] Complete commit "Re-Fix GROUP BY's columns handling"
(https://github.com/gmantele/taplib/commit/7a70c6038cef460ab169682bed391bb5ae1de1e9)
It was not possible to use a GROUP BY with a qualified column name.
So finally, now, a GROUP BY is a ClauseADQL<ADQLColumn> instead of
a ClauseADQL<ColumnReference>. Indeed, according to the ADQL's BNF,
GROUP BY items are only columns as they would appear in the SELECT
clause (i.e. qualified or not, delimited or not). On the other
hand an ORDER BY accepts ONLY column index or non-qualified column
name/alias.
The class ColumnReference is kept for backward compatibility (or in
case the next version of the ADQL grammar make items of GROUP BY and
ORDER BY of the same type: index or qualified column). Besides, this
class is still inherited for the ORDER BY clause items
(see adql.query.ADQLOrder).
---
src/adql/db/DBChecker.java | 4 +-
src/adql/parser/ADQLParser.java | 319 +++++++++++++---------------
src/adql/parser/adqlGrammar.jj | 17 +-
src/adql/query/ADQLQuery.java | 14 +-
src/adql/query/ColumnReference.java | 7 +-
test/adql/db/TestDBChecker.java | 16 ++
6 files changed, 182 insertions(+), 195 deletions(-)
diff --git a/src/adql/db/DBChecker.java b/src/adql/db/DBChecker.java
index 297de82..8e6b8ae 100644
--- a/src/adql/db/DBChecker.java
+++ b/src/adql/db/DBChecker.java
@@ -97,7 +97,7 @@ import adql.search.SimpleSearchHandler;
* </i></p>
*
* @author Grégory Mantelet (CDS;ARI)
- * @version 1.4 (03/2017)
+ * @version 1.4 (04/2017)
*/
public class DBChecker implements QueryChecker {
@@ -632,7 +632,7 @@ public class DBChecker implements QueryChecker {
// Check the correctness of all column references (= references to selected columns):
/* Note: no need to provide the father tables when resolving column references,
- * because no father column can be used in ORDER BY and/or GROUP BY. */
+ * because no father column can be used in ORDER BY. */
sHandler = new SearchColReferenceHandler();
sHandler.search(query);
ClauseSelect select = query.getSelect();
diff --git a/src/adql/parser/ADQLParser.java b/src/adql/parser/ADQLParser.java
index 8da4035..ce4f24d 100644
--- a/src/adql/parser/ADQLParser.java
+++ b/src/adql/parser/ADQLParser.java
@@ -16,7 +16,6 @@ import adql.query.ADQLQuery;
import adql.query.ClauseADQL;
import adql.query.ClauseConstraints;
import adql.query.ClauseSelect;
-import adql.query.ColumnReference;
import adql.query.SelectAllColumns;
import adql.query.SelectItem;
import adql.query.TextPosition;
@@ -889,11 +888,11 @@ public class ADQLParser implements ADQLParserConstants {
final public void GroupBy() throws ParseException{
trace_call("GroupBy");
try{
- ClauseADQL<ColumnReference> groupBy = query.getGroupBy();
- ColumnReference colRef = null;
+ ClauseADQL<ADQLColumn> groupBy = query.getGroupBy();
+ ADQLColumn colRef = null;
Token start;
start = jj_consume_token(GROUP_BY);
- colRef = ColumnRef();
+ colRef = Column();
groupBy.add(colRef);
label_3: while(true){
switch((jj_ntk == -1) ? jj_ntk_f() : jj_ntk){
@@ -906,7 +905,7 @@ public class ADQLParser implements ADQLParserConstants {
break label_3;
}
jj_consume_token(COMMA);
- colRef = ColumnRef();
+ colRef = Column();
groupBy.add(colRef);
}
groupBy.setPosition(new TextPosition(start.beginLine, start.beginColumn, colRef.getPosition().endLine, colRef.getPosition().endColumn));
@@ -1093,28 +1092,6 @@ public class ADQLParser implements ADQLParserConstants {
}
}
- final public ColumnReference ColumnRef() throws ParseException{
- trace_call("ColumnRef");
- try{
- IdentifierItems identifiers = null;
- identifiers = ColumnName();
- try{
- {
- if ("" != null)
- return queryFactory.createColRef(identifiers);
- }
- }catch(Exception ex){
- {
- if (true)
- throw generateParseException(ex);
- }
- }
- throw new Error("Missing return statement in function");
- }finally{
- trace_return("ColumnRef");
- }
- }
-
final public ADQLOrder OrderItem() throws ParseException{
trace_call("OrderItem");
try{
@@ -3935,6 +3912,72 @@ public class ADQLParser implements ADQLParserConstants {
}
}
+ private boolean jj_3R_59(){
+ if (jj_3R_24())
+ return true;
+ return false;
+ }
+
+ private boolean jj_3R_58(){
+ if (jj_scan_token(LEFT_PAR))
+ return true;
+ if (jj_3R_42())
+ return true;
+ if (jj_scan_token(RIGHT_PAR))
+ return true;
+ return false;
+ }
+
+ private boolean jj_3R_57(){
+ if (jj_3R_27())
+ return true;
+ return false;
+ }
+
+ private boolean jj_3R_124(){
+ if (jj_scan_token(CIRCLE))
+ return true;
+ if (jj_scan_token(LEFT_PAR))
+ return true;
+ if (jj_3R_134())
+ return true;
+ if (jj_scan_token(COMMA))
+ return true;
+ if (jj_3R_135())
+ return true;
+ if (jj_scan_token(COMMA))
+ return true;
+ if (jj_3R_101())
+ return true;
+ if (jj_scan_token(RIGHT_PAR))
+ return true;
+ return false;
+ }
+
+ private boolean jj_3R_44(){
+ if (jj_scan_token(SELECT))
+ return true;
+ return false;
+ }
+
+ private boolean jj_3R_56(){
+ if (jj_3R_101())
+ return true;
+ return false;
+ }
+
+ private boolean jj_3R_123(){
+ if (jj_scan_token(CENTROID))
+ return true;
+ if (jj_scan_token(LEFT_PAR))
+ return true;
+ if (jj_3R_113())
+ return true;
+ if (jj_scan_token(RIGHT_PAR))
+ return true;
+ return false;
+ }
+
private boolean jj_3R_117(){
if (jj_scan_token(LEFT_PAR))
return true;
@@ -3945,6 +3988,14 @@ public class ADQLParser implements ADQLParserConstants {
return false;
}
+ private boolean jj_3R_16(){
+ if (jj_scan_token(LEFT_PAR))
+ return true;
+ if (jj_3R_31())
+ return true;
+ return false;
+ }
+
private boolean jj_3R_122(){
if (jj_scan_token(BOX))
return true;
@@ -4004,12 +4055,6 @@ public class ADQLParser implements ADQLParserConstants {
return false;
}
- private boolean jj_3R_44(){
- if (jj_scan_token(SELECT))
- return true;
- return false;
- }
-
private boolean jj_3R_115(){
if (jj_3R_22())
return true;
@@ -4034,14 +4079,6 @@ public class ADQLParser implements ADQLParserConstants {
return false;
}
- private boolean jj_3R_16(){
- if (jj_scan_token(LEFT_PAR))
- return true;
- if (jj_3R_31())
- return true;
- return false;
- }
-
private boolean jj_3R_102(){
Token xsp;
xsp = jj_scanpos;
@@ -4166,6 +4203,12 @@ public class ADQLParser implements ADQLParserConstants {
return false;
}
+ private boolean jj_3R_31(){
+ if (jj_3R_44())
+ return true;
+ return false;
+ }
+
private boolean jj_3R_100(){
if (jj_scan_token(DISTANCE))
return true;
@@ -4237,12 +4280,6 @@ public class ADQLParser implements ADQLParserConstants {
return false;
}
- private boolean jj_3R_31(){
- if (jj_3R_44())
- return true;
- return false;
- }
-
private boolean jj_3R_104(){
if (jj_scan_token(RIGHT))
return true;
@@ -4639,6 +4676,16 @@ public class ADQLParser implements ADQLParserConstants {
return false;
}
+ private boolean jj_3R_36(){
+ if (jj_3R_14())
+ return true;
+ Token xsp;
+ xsp = jj_scanpos;
+ if (jj_3R_52())
+ jj_scanpos = xsp;
+ return false;
+ }
+
private boolean jj_3R_114(){
if (jj_3R_42())
return true;
@@ -4653,44 +4700,34 @@ public class ADQLParser implements ADQLParserConstants {
return false;
}
- private boolean jj_3R_142(){
- if (jj_scan_token(COMMA))
+ private boolean jj_3R_119(){
+ if (jj_scan_token(DOT))
return true;
- if (jj_3R_152())
+ if (jj_3R_14())
return true;
return false;
}
- private boolean jj_3R_141(){
- if (jj_scan_token(COMMA))
- return true;
- if (jj_3R_152())
+ private boolean jj_3R_118(){
+ if (jj_scan_token(DOT))
return true;
- return false;
- }
-
- private boolean jj_3R_36(){
if (jj_3R_14())
return true;
- Token xsp;
- xsp = jj_scanpos;
- if (jj_3R_52())
- jj_scanpos = xsp;
return false;
}
- private boolean jj_3R_119(){
- if (jj_scan_token(DOT))
+ private boolean jj_3R_142(){
+ if (jj_scan_token(COMMA))
return true;
- if (jj_3R_14())
+ if (jj_3R_152())
return true;
return false;
}
- private boolean jj_3R_118(){
- if (jj_scan_token(DOT))
+ private boolean jj_3R_141(){
+ if (jj_scan_token(COMMA))
return true;
- if (jj_3R_14())
+ if (jj_3R_152())
return true;
return false;
}
@@ -4720,6 +4757,17 @@ public class ADQLParser implements ADQLParserConstants {
return false;
}
+ private boolean jj_3R_14(){
+ Token xsp;
+ xsp = jj_scanpos;
+ if (jj_3R_28()){
+ jj_scanpos = xsp;
+ if (jj_3R_29())
+ return true;
+ }
+ return false;
+ }
+
private boolean jj_3R_131(){
if (jj_3R_102())
return true;
@@ -4746,17 +4794,6 @@ public class ADQLParser implements ADQLParserConstants {
return false;
}
- private boolean jj_3R_14(){
- Token xsp;
- xsp = jj_scanpos;
- if (jj_3R_28()){
- jj_scanpos = xsp;
- if (jj_3R_29())
- return true;
- }
- return false;
- }
-
private boolean jj_3R_130(){
if (jj_3R_21())
return true;
@@ -5145,6 +5182,14 @@ public class ADQLParser implements ADQLParserConstants {
return false;
}
+ private boolean jj_3R_30(){
+ if (jj_3R_14())
+ return true;
+ if (jj_scan_token(DOT))
+ return true;
+ return false;
+ }
+
private boolean jj_3R_27(){
if (jj_3R_35())
return true;
@@ -5227,16 +5272,20 @@ public class ADQLParser implements ADQLParserConstants {
return false;
}
- private boolean jj_3R_39(){
- if (jj_3R_54())
+ private boolean jj_3R_15(){
+ if (jj_3R_14())
return true;
+ if (jj_scan_token(DOT))
+ return true;
+ Token xsp;
+ xsp = jj_scanpos;
+ if (jj_3R_30())
+ jj_scanpos = xsp;
return false;
}
- private boolean jj_3R_30(){
- if (jj_3R_14())
- return true;
- if (jj_scan_token(DOT))
+ private boolean jj_3R_39(){
+ if (jj_3R_54())
return true;
return false;
}
@@ -5281,18 +5330,6 @@ public class ADQLParser implements ADQLParserConstants {
return false;
}
- private boolean jj_3R_15(){
- if (jj_3R_14())
- return true;
- if (jj_scan_token(DOT))
- return true;
- Token xsp;
- xsp = jj_scanpos;
- if (jj_3R_30())
- jj_scanpos = xsp;
- return false;
- }
-
private boolean jj_3R_133(){
Token xsp;
xsp = jj_scanpos;
@@ -5352,6 +5389,20 @@ public class ADQLParser implements ADQLParserConstants {
return false;
}
+ private boolean jj_3_1(){
+ if (jj_3R_14())
+ return true;
+ if (jj_scan_token(DOT))
+ return true;
+ Token xsp;
+ xsp = jj_scanpos;
+ if (jj_3R_15())
+ jj_scanpos = xsp;
+ if (jj_scan_token(ASTERISK))
+ return true;
+ return false;
+ }
+
private boolean jj_3R_19(){
if (jj_3R_34())
return true;
@@ -5430,20 +5481,6 @@ public class ADQLParser implements ADQLParserConstants {
return false;
}
- private boolean jj_3_1(){
- if (jj_3R_14())
- return true;
- if (jj_scan_token(DOT))
- return true;
- Token xsp;
- xsp = jj_scanpos;
- if (jj_3R_15())
- jj_scanpos = xsp;
- if (jj_scan_token(ASTERISK))
- return true;
- return false;
- }
-
private boolean jj_3_9(){
if (jj_3R_22())
return true;
@@ -5575,66 +5612,6 @@ public class ADQLParser implements ADQLParserConstants {
return false;
}
- private boolean jj_3R_59(){
- if (jj_3R_24())
- return true;
- return false;
- }
-
- private boolean jj_3R_58(){
- if (jj_scan_token(LEFT_PAR))
- return true;
- if (jj_3R_42())
- return true;
- if (jj_scan_token(RIGHT_PAR))
- return true;
- return false;
- }
-
- private boolean jj_3R_57(){
- if (jj_3R_27())
- return true;
- return false;
- }
-
- private boolean jj_3R_124(){
- if (jj_scan_token(CIRCLE))
- return true;
- if (jj_scan_token(LEFT_PAR))
- return true;
- if (jj_3R_134())
- return true;
- if (jj_scan_token(COMMA))
- return true;
- if (jj_3R_135())
- return true;
- if (jj_scan_token(COMMA))
- return true;
- if (jj_3R_101())
- return true;
- if (jj_scan_token(RIGHT_PAR))
- return true;
- return false;
- }
-
- private boolean jj_3R_56(){
- if (jj_3R_101())
- return true;
- return false;
- }
-
- private boolean jj_3R_123(){
- if (jj_scan_token(CENTROID))
- return true;
- if (jj_scan_token(LEFT_PAR))
- return true;
- if (jj_3R_113())
- return true;
- if (jj_scan_token(RIGHT_PAR))
- return true;
- return false;
- }
-
/** Generated Token Manager. */
public ADQLParserTokenManager token_source;
SimpleCharStream jj_input_stream;
diff --git a/src/adql/parser/adqlGrammar.jj b/src/adql/parser/adqlGrammar.jj
index 857997b..a4d2cb4 100644
--- a/src/adql/parser/adqlGrammar.jj
+++ b/src/adql/parser/adqlGrammar.jj
@@ -841,9 +841,9 @@ void Where(): {ClauseConstraints where = query.getWhere(); ADQLConstraint condit
}
}
-void GroupBy(): {ClauseADQL<ColumnReference> groupBy = query.getGroupBy(); ColumnReference colRef = null; Token start;} {
- start=<GROUP_BY> colRef=ColumnRef() { groupBy.add(colRef); }
- ( <COMMA> colRef=ColumnRef() { groupBy.add(colRef); } )*
+void GroupBy(): {ClauseADQL<ADQLColumn> groupBy = query.getGroupBy(); ADQLColumn colRef = null; Token start;} {
+ start=<GROUP_BY> colRef=Column() { groupBy.add(colRef); }
+ ( <COMMA> colRef=Column() { groupBy.add(colRef); } )*
{ groupBy.setPosition(new TextPosition(start.beginLine, start.beginColumn, colRef.getPosition().endLine, colRef.getPosition().endColumn)); }
}
@@ -916,17 +916,6 @@ ADQLColumn Column(): {IdentifierItems identifiers;} {
}
}
-ColumnReference ColumnRef(): {IdentifierItems identifiers = null;}{
- identifiers=ColumnName()
- {
- try{
- return queryFactory.createColRef(identifiers);
- }catch(Exception ex){
- throw generateParseException(ex);
- }
- }
-}
-
ADQLOrder OrderItem(): {IdentifierItem identifier = null; Token ind = null, desc = null;}{
(identifier=Identifier() | ind=<UNSIGNED_INTEGER>) (<ASC> | desc=<DESC>)?
{
diff --git a/src/adql/query/ADQLQuery.java b/src/adql/query/ADQLQuery.java
index dd0a9b9..bd0d19c 100644
--- a/src/adql/query/ADQLQuery.java
+++ b/src/adql/query/ADQLQuery.java
@@ -47,7 +47,7 @@ import adql.search.ISearchHandler;
* <p>The resulting object of the {@link ADQLParser} is an object of this class.</p>
*
* @author Grégory Mantelet (CDS;ARI)
- * @version 1.4 (02/2017)
+ * @version 1.4 (04/2017)
*/
public class ADQLQuery implements ADQLObject {
@@ -61,7 +61,7 @@ public class ADQLQuery implements ADQLObject {
private ClauseConstraints where;
/** The ADQL clause GROUP BY. */
- private ClauseADQL<ColumnReference> groupBy;
+ private ClauseADQL<ADQLColumn> groupBy;
/** The ADQL clause HAVING. */
private ClauseConstraints having;
@@ -80,7 +80,7 @@ public class ADQLQuery implements ADQLObject {
select = new ClauseSelect();
from = null;
where = new ClauseConstraints("WHERE");
- groupBy = new ClauseADQL<ColumnReference>("GROUP BY");
+ groupBy = new ClauseADQL<ADQLColumn>("GROUP BY");
having = new ClauseConstraints("HAVING");
orderBy = new ClauseADQL<ADQLOrder>("ORDER BY");
}
@@ -96,7 +96,7 @@ public class ADQLQuery implements ADQLObject {
select = (ClauseSelect)toCopy.select.getCopy();
from = (FromContent)toCopy.from.getCopy();
where = (ClauseConstraints)toCopy.where.getCopy();
- groupBy = (ClauseADQL<ColumnReference>)toCopy.groupBy.getCopy();
+ groupBy = (ClauseADQL<ADQLColumn>)toCopy.groupBy.getCopy();
having = (ClauseConstraints)toCopy.having.getCopy();
orderBy = (ClauseADQL<ADQLOrder>)toCopy.orderBy.getCopy();
position = (toCopy.position == null) ? null : new TextPosition(toCopy.position);
@@ -201,7 +201,7 @@ public class ADQLQuery implements ADQLObject {
*
* @return Its GROUP BY clause.
*/
- public final ClauseADQL<ColumnReference> getGroupBy(){
+ public final ClauseADQL<ADQLColumn> getGroupBy(){
return groupBy;
}
@@ -213,7 +213,7 @@ public class ADQLQuery implements ADQLObject {
* @param newGroupBy The new GROUP BY clause.
* @throws NullPointerException If the given GROUP BY clause is <i>null</i>.
*/
- public void setGroupBy(ClauseADQL<ColumnReference> newGroupBy) throws NullPointerException{
+ public void setGroupBy(ClauseADQL<ADQLColumn> newGroupBy) throws NullPointerException{
if (newGroupBy == null)
groupBy.clear();
else
@@ -454,7 +454,7 @@ public class ADQLQuery implements ADQLObject {
break;
case 3:
if (replacer instanceof ClauseADQL)
- groupBy = (ClauseADQL<ColumnReference>)replacer;
+ groupBy = (ClauseADQL<ADQLColumn>)replacer;
else
throw new UnsupportedOperationException("Impossible to replace a ClauseADQL (" + groupBy.toADQL() + ") by a " + replacer.getClass().getName() + " (" + replacer.toADQL() + ") !");
break;
diff --git a/src/adql/query/ColumnReference.java b/src/adql/query/ColumnReference.java
index 3390e7a..78d41c2 100644
--- a/src/adql/query/ColumnReference.java
+++ b/src/adql/query/ColumnReference.java
@@ -24,7 +24,7 @@ import adql.query.from.ADQLTable;
import adql.query.operand.ADQLColumn;
/**
- * Represents a reference to a selected column either by an index or by a name/alias.
+ * Represents a reference to a selected column either by an index or by a non-qualified column name/alias.
*
* @author Grégory Mantelet (CDS)
* @version 01/2012
@@ -94,6 +94,7 @@ public class ColumnReference implements ADQLObject {
*
* @return The position of this {@link ColumnReference}.
*/
+ @Override
public final TextPosition getPosition(){
return position;
}
@@ -235,18 +236,22 @@ public class ColumnReference implements ADQLObject {
this.adqlTable = adqlTable;
}
+ @Override
public ADQLObject getCopy() throws Exception{
return new ColumnReference(this);
}
+ @Override
public String getName(){
return isIndex() ? (columnIndex + "") : columnName;
}
+ @Override
public final ADQLIterator adqlIterator(){
return new NullADQLIterator();
}
+ @Override
public String toADQL(){
return isIndex() ? ("" + columnIndex) : (isCaseSensitive() ? ("\"" + columnName + "\"") : columnName);
}
diff --git a/test/adql/db/TestDBChecker.java b/test/adql/db/TestDBChecker.java
index b0b184b..aec904d 100644
--- a/test/adql/db/TestDBChecker.java
+++ b/test/adql/db/TestDBChecker.java
@@ -96,6 +96,22 @@ public class TestDBChecker {
assertEquals(expected[i], names[i]);
}
+ @Test
+ public void testGroupByWithQualifiedColName(){
+ ADQLParser parser = new ADQLParser(new DBChecker(tables, new ArrayList<FunctionDef>(0)));
+ try{
+ // Not qualified column name:
+ parser.parseQuery("SELECT colI, COUNT(*) AS cnt FROM foo GROUP BY colI");
+ // Qualified with the table name:
+ parser.parseQuery("SELECT foo.colI, COUNT(*) AS cnt FROM foo GROUP BY foo.colI");
+ // Qualified with the table alias:
+ parser.parseQuery("SELECT f.colI, COUNT(*) AS cnt FROM foo AS f GROUP BY f.colI");
+ }catch(ParseException pe){
+ pe.printStackTrace();
+ fail();
+ }
+ }
+
@Test
public void testQualifiedName(){
ADQLParser parser = new ADQLParser(new DBChecker(tables, new ArrayList<FunctionDef>(0)));
--
GitLab