From 271e03cc5e1fcaaff775558bda300e7f1ab3b906 Mon Sep 17 00:00:00 2001
From: gmantele <gmantele@ari.uni-heidelberg.de>
Date: Tue, 1 Sep 2015 16:08:30 +0200
Subject: [PATCH] [ADQL,TAP] Fix bug (reported by G. Landais) in the
understanding of UNKNOWN types. The notion of "unknown type" is different in
function of the target object: - a DBType and a FunctionDef have an unknown
type if their function isUnknown() returns true. In such case, the other
functions such as isNumeric/String/Geometry() will return false. - an
ADQLOperand (e.g. ADQLColumn) does NOT have a isUnknown() function. But
if the type of the operand is unknown, its functions isNumeric(),
isString() and isGeometry() must ALL return true. Otherwise, just one of
these functions can return true.
---
src/adql/db/DBType.java | 19 +++++------
src/adql/db/FunctionDef.java | 23 ++++++++++----
src/adql/query/operand/ADQLColumn.java | 6 ++--
src/tap/formatter/JSONFormat.java | 4 +--
src/tap/formatter/VOTableFormat.java | 2 +-
test/adql/db/TestFunctionDef.java | 27 ++++++++--------
test/adql/parser/UnknownTypes.java | 44 ++++++++++++++++++++------
7 files changed, 80 insertions(+), 45 deletions(-)
diff --git a/src/adql/db/DBType.java b/src/adql/db/DBType.java
index 18a09e4..ca27139 100644
--- a/src/adql/db/DBType.java
+++ b/src/adql/db/DBType.java
@@ -32,7 +32,7 @@ package adql.db;
* It is used to set the attribute type/datatype of this class.</p>
*
* @author Grégory Mantelet (ARI)
- * @version 1.4 (07/2015)
+ * @version 1.4 (08/2015)
* @since 1.3
*/
public class DBType {
@@ -119,7 +119,8 @@ public class DBType {
*
* <p><i><b>Important note</b>:
* Since {@link DBDatatype#UNKNOWN UNKNOWN} is an unresolved type, it can potentially be anything.
- * That's why, this function will also returned <code>true</code> if the type is
+ * But, in order to avoid incorrect operation while expecting a numeric although the type is unknown
+ * and is in fact not really a numeric, this function will return <code>false</code> if the type is
* {@link DBDatatype#UNKNOWN UNKNOWN}.
* </i></p>
*
@@ -137,7 +138,6 @@ public class DBType {
case BINARY:
case VARBINARY:
case BLOB:
- case UNKNOWN:
return true;
default:
return false;
@@ -153,7 +153,8 @@ public class DBType {
*
* <p><i><b>Important note</b>:
* Since {@link DBDatatype#UNKNOWN UNKNOWN} is an unresolved type, it can potentially be anything.
- * That's why, this function will also returned <code>true</code> if the type is
+ * But, in order to avoid incorrect operation while expecting a binary although the type is unknown
+ * and is in fact not really a binary, this function will return <code>false</code> if the type is
* {@link DBDatatype#UNKNOWN UNKNOWN}.
* </i></p>
*
@@ -164,7 +165,6 @@ public class DBType {
case BINARY:
case VARBINARY:
case BLOB:
- case UNKNOWN:
return true;
default:
return false;
@@ -181,7 +181,8 @@ public class DBType {
*
* <p><i><b>Important note</b>:
* Since {@link DBDatatype#UNKNOWN UNKNOWN} is an unresolved type, it can potentially be anything.
- * That's why, this function will also returned <code>true</code> if the type is
+ * But, in order to avoid incorrect operation while expecting a string although the type is unknown
+ * and is in fact not really a string, this function will return <code>false</code> if the type is
* {@link DBDatatype#UNKNOWN UNKNOWN}.
* </i></p>
*
@@ -193,7 +194,6 @@ public class DBType {
case VARCHAR:
case CLOB:
case TIMESTAMP:
- case UNKNOWN:
return true;
default:
return false;
@@ -209,14 +209,15 @@ public class DBType {
*
* <p><i><b>Important note</b>:
* Since {@link DBDatatype#UNKNOWN UNKNOWN} is an unresolved type, it can potentially be anything.
- * That's why, this function will also returned <code>true</code> if the type is
+ * But, in order to avoid incorrect operation while expecting a geometry although the type is unknown
+ * and is in fact not really a geometry, this function will return <code>false</code> if the type is
* {@link DBDatatype#UNKNOWN UNKNOWN}.
* </i></p>
*
* @return <code>true</code> if this type is a geometry, <code>false</code> otherwise.
*/
public boolean isGeometry(){
- return (type == DBDatatype.POINT || type == DBDatatype.REGION || type == DBDatatype.UNKNOWN);
+ return (type == DBDatatype.POINT || type == DBDatatype.REGION);
}
/**
diff --git a/src/adql/db/FunctionDef.java b/src/adql/db/FunctionDef.java
index 883de81..b6a160b 100644
--- a/src/adql/db/FunctionDef.java
+++ b/src/adql/db/FunctionDef.java
@@ -201,9 +201,9 @@ public class FunctionDef implements Comparable<FunctionDef> {
// Set the return type;
this.returnType = (returnType != null) ? returnType : new DBType(DBDatatype.UNKNOWN);
isUnknown = this.returnType.isUnknown();
- isNumeric = isUnknown || this.returnType.isNumeric();
- isString = isUnknown || this.returnType.isString();
- isGeometry = isUnknown || this.returnType.isGeometry();
+ isNumeric = this.returnType.isNumeric();
+ isString = this.returnType.isString();
+ isGeometry = this.returnType.isGeometry();
// Serialize in Strings (serializedForm and compareForm) this function definition:
StringBuffer bufSer = new StringBuffer(name), bufCmp = new StringBuffer(name.toLowerCase());
@@ -515,7 +515,7 @@ public class FunctionDef implements Comparable<FunctionDef> {
* not part of a function signature.
* </p>
*
- * <p>The notion of "greater" and "less" are defined here according to the three following test steps:</p>
+ * <p>The notions of "greater" and "less" are defined here according to the three following test steps:</p>
* <ol>
* <li><b>Name test:</b> if the name of both function are equals, next steps are evaluated, otherwise the standard string comparison (case insensitive) result is returned.</li>
* <li><b>Parameters test:</b> parameters are compared individually. Each time parameters (at the same position in both functions) are equals the next parameter can be tested,
@@ -529,10 +529,19 @@ public class FunctionDef implements Comparable<FunctionDef> {
* returned. Otherwise a negative value will be returned, or 0 if the number of parameters is the same.</li>
* </ol>
*
+ * <p><i><b>Note:</b>
+ * If one of the tested types (i.e. parameters types) is unknown, the match should return 0 (i.e. equality).
+ * The notion of "unknown" is different in function of the tested item. A {@link DBType} is unknown if its function
+ * {@link DBType#isUnknown()} returns <code>true</code> ; thus, its other functions such as {@link DBType#isNumeric()} will
+ * return <code>false</code>. On the contrary, an {@link ADQLOperand} does not have any isUnknown()
+ * function. However, when the type of a such is unknown, all its functions isNumeric(), isString() and isGeometry() return
+ * <code>true</code>.
+ * </i></p>
+ *
* @param fct ADQL function item to compare with this function definition.
*
* @return A positive value if this function definition is "greater" than the given {@link ADQLFunction},
- * 0 if they are perfectly matching,
+ * 0 if they are perfectly matching or one of the tested types (i.e. parameters types) is unknown,
* or a negative value if this function definition is "less" than the given {@link ADQLFunction}.
*/
public int compareTo(final ADQLFunction fct){
@@ -545,8 +554,10 @@ public class FunctionDef implements Comparable<FunctionDef> {
// If equals, compare the parameters' type:
if (comp == 0){
for(int i = 0; comp == 0 && i < nbParams && i < fct.getNbParameters(); i++){
- if (fct.getParameter(i).isNumeric() && fct.getParameter(i).isString() && fct.getParameter(i).isGeometry())
+ // if one of the types is unknown, the comparison should return true:
+ if (params[i].type.isUnknown() || (fct.getParameter(i).isNumeric() && fct.getParameter(i).isString() && fct.getParameter(i).isGeometry()))
comp = 0;
+ // otherwise, just compare each kind of type for an exact match:
else if (params[i].type.isNumeric() == fct.getParameter(i).isNumeric()){
if (params[i].type.isString() == fct.getParameter(i).isString()){
if (params[i].type.isGeometry() == fct.getParameter(i).isGeometry())
diff --git a/src/adql/query/operand/ADQLColumn.java b/src/adql/query/operand/ADQLColumn.java
index eebce23..af8e9d5 100644
--- a/src/adql/query/operand/ADQLColumn.java
+++ b/src/adql/query/operand/ADQLColumn.java
@@ -469,17 +469,17 @@ public class ADQLColumn implements ADQLOperand, UnknownType {
@Override
public boolean isNumeric(){
- return (dbLink == null || dbLink.getDatatype() == null || dbLink.getDatatype().isNumeric());
+ return (dbLink == null || dbLink.getDatatype() == null || dbLink.getDatatype().isUnknown() || dbLink.getDatatype().isNumeric());
}
@Override
public boolean isString(){
- return (dbLink == null || dbLink.getDatatype() == null || dbLink.getDatatype().isString());
+ return (dbLink == null || dbLink.getDatatype() == null || dbLink.getDatatype().isUnknown() || dbLink.getDatatype().isString());
}
@Override
public boolean isGeometry(){
- return (dbLink == null || dbLink.getDatatype() == null || dbLink.getDatatype().isGeometry());
+ return (dbLink == null || dbLink.getDatatype() == null || dbLink.getDatatype().isUnknown() || dbLink.getDatatype().isGeometry());
}
@Override
diff --git a/src/tap/formatter/JSONFormat.java b/src/tap/formatter/JSONFormat.java
index 515e43a..7481230 100644
--- a/src/tap/formatter/JSONFormat.java
+++ b/src/tap/formatter/JSONFormat.java
@@ -42,7 +42,7 @@ import adql.db.DBType.DBDatatype;
* Format any given query (table) result into JSON.
*
* @author Grégory Mantelet (CDS;ARI)
- * @version 2.1 (07/2015)
+ * @version 2.1 (08/2015)
*/
public class JSONFormat implements OutputFormat {
@@ -180,7 +180,7 @@ public class JSONFormat implements OutputFormat {
protected TAPColumn getValidColMeta(final DBColumn typeFromQuery, final TAPColumn typeFromResult){
if (typeFromQuery != null && typeFromQuery instanceof TAPColumn){
TAPColumn colMeta = (TAPColumn)typeFromQuery;
- if (colMeta.getDatatype().type == DBDatatype.UNKNOWN && typeFromResult != null && typeFromResult.getDatatype().type != DBDatatype.UNKNOWN)
+ if (colMeta.getDatatype().isUnknown() && typeFromResult != null && !typeFromResult.getDatatype().isUnknown())
colMeta.setDatatype(typeFromResult.getDatatype());
return colMeta;
}else if (typeFromResult != null){
diff --git a/src/tap/formatter/VOTableFormat.java b/src/tap/formatter/VOTableFormat.java
index 6156d21..07e7d39 100644
--- a/src/tap/formatter/VOTableFormat.java
+++ b/src/tap/formatter/VOTableFormat.java
@@ -464,7 +464,7 @@ public class VOTableFormat implements OutputFormat {
protected static final TAPColumn getValidColMeta(final DBColumn typeFromQuery, final TAPColumn typeFromResult){
if (typeFromQuery != null && typeFromQuery instanceof TAPColumn){
TAPColumn colMeta = (TAPColumn)typeFromQuery;
- if (colMeta.getDatatype().type == DBDatatype.UNKNOWN && typeFromResult != null && typeFromResult.getDatatype().type != DBDatatype.UNKNOWN)
+ if (colMeta.getDatatype().isUnknown() && typeFromResult != null && !typeFromResult.getDatatype().isUnknown())
colMeta.setDatatype(typeFromResult.getDatatype());
return colMeta;
}else if (typeFromResult != null){
diff --git a/test/adql/db/TestFunctionDef.java b/test/adql/db/TestFunctionDef.java
index 1c8eb4d..e9ea725 100644
--- a/test/adql/db/TestFunctionDef.java
+++ b/test/adql/db/TestFunctionDef.java
@@ -27,7 +27,6 @@ public class TestFunctionDef {
case VARCHAR:
case TIMESTAMP:
case CLOB:
- case UNKNOWN:
assertTrue(new FunctionDef("foo", new DBType(type)).isString);
break;
default:
@@ -42,7 +41,6 @@ public class TestFunctionDef {
switch(type){
case POINT:
case REGION:
- case UNKNOWN:
assertTrue(new FunctionDef("foo", new DBType(type)).isGeometry);
break;
default:
@@ -61,6 +59,7 @@ public class TestFunctionDef {
case POINT:
case REGION:
case CLOB:
+ case UNKNOWN:
assertFalse(new FunctionDef("foo", new DBType(type)).isNumeric);
break;
default:
@@ -188,9 +187,9 @@ public class TestFunctionDef {
try{
FunctionDef fct = FunctionDef.parse("foo()->aType");
assertTrue(fct.isUnknown);
- assertTrue(fct.isString);
- assertTrue(fct.isNumeric);
- assertTrue(fct.isGeometry);
+ assertFalse(fct.isString);
+ assertFalse(fct.isNumeric);
+ assertFalse(fct.isGeometry);
assertEquals("?aType?", fct.returnType.type.toString());
}catch(Exception ex){
ex.printStackTrace(System.err);
@@ -199,9 +198,9 @@ public class TestFunctionDef {
try{
FunctionDef fct = FunctionDef.parse("foo()->aType(10)");
assertTrue(fct.isUnknown);
- assertTrue(fct.isString);
- assertTrue(fct.isNumeric);
- assertTrue(fct.isGeometry);
+ assertFalse(fct.isString);
+ assertFalse(fct.isNumeric);
+ assertFalse(fct.isGeometry);
assertEquals("?aType(10)?", fct.returnType.type.toString());
}catch(Exception ex){
ex.printStackTrace(System.err);
@@ -231,9 +230,9 @@ public class TestFunctionDef {
try{
FunctionDef fct = FunctionDef.parse("foo(param1 aType)");
assertTrue(fct.getParam(0).type.isUnknown());
- assertTrue(fct.getParam(0).type.isString());
- assertTrue(fct.getParam(0).type.isNumeric());
- assertTrue(fct.getParam(0).type.isGeometry());
+ assertFalse(fct.getParam(0).type.isString());
+ assertFalse(fct.getParam(0).type.isNumeric());
+ assertFalse(fct.getParam(0).type.isGeometry());
assertEquals("?aType?", fct.getParam(0).type.toString());
}catch(Exception ex){
ex.printStackTrace(System.err);
@@ -242,9 +241,9 @@ public class TestFunctionDef {
try{
FunctionDef fct = FunctionDef.parse("foo(param1 aType(10))");
assertTrue(fct.getParam(0).type.isUnknown());
- assertTrue(fct.getParam(0).type.isString());
- assertTrue(fct.getParam(0).type.isNumeric());
- assertTrue(fct.getParam(0).type.isGeometry());
+ assertFalse(fct.getParam(0).type.isString());
+ assertFalse(fct.getParam(0).type.isNumeric());
+ assertFalse(fct.getParam(0).type.isGeometry());
assertEquals("?aType(10)?", fct.getParam(0).type.toString());
}catch(Exception ex){
ex.printStackTrace(System.err);
diff --git a/test/adql/parser/UnknownTypes.java b/test/adql/parser/UnknownTypes.java
index 0ba4e0b..3ecd60d 100644
--- a/test/adql/parser/UnknownTypes.java
+++ b/test/adql/parser/UnknownTypes.java
@@ -1,6 +1,7 @@
package adql.parser;
import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;
@@ -44,9 +45,9 @@ public class UnknownTypes {
try{
FunctionDef fct = FunctionDef.parse("foo()->aType");
assertTrue(fct.isUnknown());
- assertTrue(fct.isString());
- assertTrue(fct.isNumeric());
- assertTrue(fct.isGeometry());
+ assertFalse(fct.isString());
+ assertFalse(fct.isNumeric());
+ assertFalse(fct.isGeometry());
assertEquals("?aType?", fct.returnType.type.toString());
}catch(Exception ex){
ex.printStackTrace(System.err);
@@ -57,9 +58,9 @@ public class UnknownTypes {
try{
FunctionDef fct = FunctionDef.parse("foo(param1 aType)");
assertTrue(fct.getParam(0).type.isUnknown());
- assertTrue(fct.getParam(0).type.isString());
- assertTrue(fct.getParam(0).type.isNumeric());
- assertTrue(fct.getParam(0).type.isGeometry());
+ assertFalse(fct.getParam(0).type.isString());
+ assertFalse(fct.getParam(0).type.isNumeric());
+ assertFalse(fct.getParam(0).type.isGeometry());
assertEquals("?aType?", fct.getParam(0).type.toString());
}catch(Exception ex){
ex.printStackTrace(System.err);
@@ -69,7 +70,7 @@ public class UnknownTypes {
@Test
public void testForColumns(){
- final String QUERY_TXT = "SELECT FOO(C1), FOO(C2) FROM T1";
+ final String QUERY_TXT = "SELECT FOO(C1), FOO(C2), C1, C2, C3 FROM T1";
try{
// Create the parser:
@@ -79,6 +80,7 @@ public class UnknownTypes {
DefaultDBTable table1 = new DefaultDBTable("T1");
table1.addColumn(new DefaultDBColumn("C1", table1));
table1.addColumn(new DefaultDBColumn("C2", new DBType(DBDatatype.UNKNOWN), table1));
+ table1.addColumn(new DefaultDBColumn("C3", new DBType(DBDatatype.VARCHAR), table1));
Collection<DBTable> tList = Arrays.asList(new DBTable[]{table1});
// Check the type of the column T1.C1:
@@ -91,9 +93,9 @@ public class UnknownTypes {
assertNotNull(col);
assertNotNull(col.getDatatype());
assertTrue(col.getDatatype().isUnknown());
- assertTrue(col.getDatatype().isNumeric());
- assertTrue(col.getDatatype().isString());
- assertTrue(col.getDatatype().isGeometry());
+ assertFalse(col.getDatatype().isNumeric());
+ assertFalse(col.getDatatype().isString());
+ assertFalse(col.getDatatype().isGeometry());
assertEquals("UNKNOWN", col.getDatatype().toString());
// Define a UDF, and allow all geometrical functions and coordinate systems:
@@ -110,6 +112,28 @@ public class UnknownTypes {
// Check the parsed query:
checker.check(pq);
+
+ /* Ensure the type of every ADQLColumn is as expected: */
+ // isNumeric() = true for FOO(C1), but false for the others
+ assertTrue(pq.getSelect().get(0).getOperand().isNumeric());
+ assertFalse(pq.getSelect().get(0).getOperand().isString());
+ assertFalse(pq.getSelect().get(0).getOperand().isGeometry());
+ // isNumeric() = true for FOO(C2), but false for the others
+ assertTrue(pq.getSelect().get(1).getOperand().isNumeric());
+ assertFalse(pq.getSelect().get(1).getOperand().isString());
+ assertFalse(pq.getSelect().get(1).getOperand().isGeometry());
+ // isNumeric() = isString() = isGeometry() for C1
+ assertTrue(pq.getSelect().get(2).getOperand().isNumeric());
+ assertTrue(pq.getSelect().get(2).getOperand().isString());
+ assertTrue(pq.getSelect().get(2).getOperand().isGeometry());
+ // isNumeric() = isString() = isGeometry() for C2
+ assertTrue(pq.getSelect().get(3).getOperand().isNumeric());
+ assertTrue(pq.getSelect().get(3).getOperand().isString());
+ assertTrue(pq.getSelect().get(3).getOperand().isGeometry());
+ // isString() = true for C3, but false for the others
+ assertFalse(pq.getSelect().get(4).getOperand().isNumeric());
+ assertTrue(pq.getSelect().get(4).getOperand().isString());
+ assertFalse(pq.getSelect().get(4).getOperand().isGeometry());
}catch(Exception ex){
ex.printStackTrace(System.err);
fail("The construction, configuration and usage of the parser are correct. Nothing should have failed here. (see console for more details)");
--
GitLab