From ad2acca3c5a24c968ffa6e27b9a49e4a6eae7f86 Mon Sep 17 00:00:00 2001
From: gmantele <gmantele@ari.uni-heidelberg.de>
Date: Wed, 25 May 2016 23:05:46 +0200
Subject: [PATCH] [ADQL] Fix recursive replacement using SimpleReplaceHandler.
Before correction, if an ADQlObject (e.g. a function or a sub-query) contains
another ADQLObject and that both (i.e. parent and child) are matching in a
SimpleReplaceHandler and are asked to be replaced, only the parent seemed to
have been replaced. However, the child has been replaced, but in the former
instance of the parent ; and so its replacement is not visible in the final
query.
For instance:
if all mathematical functions must be replaced by a dumb UDF named 'foo' in
the ADQL query:
"SELECT sqrt(abs(81)) FROM myTable"
,the result should be:
"SELECT foo(foo(81)) FROM myTable"
,but before this correction it was:
"SELECT foo(abs(81)) FROM myTable".
---
src/adql/search/SimpleReplaceHandler.java | 76 ++++++++++++++++---
.../adql/search/TestSimpleReplaceHandler.java | 67 ++++++++++++++++
2 files changed, 131 insertions(+), 12 deletions(-)
create mode 100644 test/adql/search/TestSimpleReplaceHandler.java
diff --git a/src/adql/search/SimpleReplaceHandler.java b/src/adql/search/SimpleReplaceHandler.java
index 7f14484..53ebc93 100644
--- a/src/adql/search/SimpleReplaceHandler.java
+++ b/src/adql/search/SimpleReplaceHandler.java
@@ -1,5 +1,7 @@
package adql.search;
+import java.util.Stack;
+
/*
* This file is part of ADQLLibrary.
*
@@ -16,7 +18,8 @@ package adql.search;
* You should have received a copy of the GNU Lesser General Public License
* along with ADQLLibrary. If not, see <http://www.gnu.org/licenses/>.
*
- * Copyright 2012 - UDS/Centre de Données astronomiques de Strasbourg (CDS)
+ * Copyright 2012,2016 - UDS/Centre de Données astronomiques de Strasbourg (CDS),
+ * Astronomisches Rechen Institut (ARI)
*/
import adql.query.ADQLIterator;
@@ -31,16 +34,13 @@ import adql.query.ADQLObject;
* <li>Matching objects are collected before their replacement.</li>
* </ul>
*
- * @author Grégory Mantelet (CDS)
- * @version 06/2011
+ * @author Grégory Mantelet (CDS;ARI)
+ * @version 1.4 (05/2016)
*
* @see RemoveHandler
*/
public abstract class SimpleReplaceHandler extends SimpleSearchHandler implements IReplaceHandler {
- /** Indicates whether {@link #searchAndReplace(ADQLObject)} (=true) has been called or just {@link #search(ADQLObject)} (=false). */
- protected boolean replaceActive = false;
-
/** Count matching objects which have been replaced successfully. */
protected int nbReplacement = 0;
@@ -74,6 +74,7 @@ public abstract class SimpleReplaceHandler extends SimpleSearchHandler implement
super(recursive, onlyFirstMatch);
}
+ @Override
public int getNbReplacement(){
return nbReplacement;
}
@@ -84,11 +85,25 @@ public abstract class SimpleReplaceHandler extends SimpleSearchHandler implement
nbReplacement = 0;
}
- @Override
- protected void addMatch(ADQLObject matchObj, ADQLIterator it){
+ /**
+ * <p>Adds the given ADQL object as one result of the research, and then replace its reference
+ * inside its parent.</p>
+ *
+ * <p>Thus, the matched item added in the list is no longer available in its former parent.</p>
+ *
+ * <p><b><u>Warning:</u> the second parameter (<i>it</i>) may be <i>null</i> if the given match is the root search object itself.</b></p>
+ *
+ * @param matchObj An ADQL object which has matched to the research criteria.
+ * @param it The iterator from which the matched ADQL object has been extracted.
+ *
+ * @return The match item after replacement if any replacement has occurred,
+ * or <code>null</code> if the item has been removed,
+ * or the object given in parameter if there was no replacement.
+ */
+ protected ADQLObject addMatchAndReplace(ADQLObject matchObj, ADQLIterator it){
super.addMatch(matchObj, it);
- if (replaceActive && it != null){
+ if (it != null){
try{
ADQLObject replacer = getReplacer(matchObj);
if (replacer == null)
@@ -96,18 +111,55 @@ public abstract class SimpleReplaceHandler extends SimpleSearchHandler implement
else
it.replace(replacer);
nbReplacement++;
+ return replacer;
}catch(IllegalStateException ise){
}catch(UnsupportedOperationException uoe){
}
}
+
+ return matchObj;
}
+ @Override
public void searchAndReplace(final ADQLObject startObj){
- replaceActive = true;
- search(startObj);
- replaceActive = false;
+ reset();
+
+ if (startObj == null)
+ return;
+
+ // Test the root search object:
+ if (match(startObj))
+ addMatch(startObj, null);
+
+ Stack<ADQLIterator> stackIt = new Stack<ADQLIterator>();
+ ADQLObject obj = null;
+ ADQLIterator it = startObj.adqlIterator();
+
+ while(!isFinished()){
+ // Fetch the next ADQL object to test:
+ do{
+ if (it != null && it.hasNext())
+ obj = it.next();
+ else if (!stackIt.isEmpty())
+ it = stackIt.pop();
+ else
+ return;
+ }while(obj == null);
+
+ // Add the current object if it is matching:
+ if (match(obj))
+ obj = addMatchAndReplace(obj, it);
+
+ // Continue the research inside the current object (or the new object if a replacement has been performed):
+ if (obj != null && goInto(obj)){
+ stackIt.push(it);
+ it = obj.adqlIterator();
+ }
+
+ obj = null;
+ }
}
/**
diff --git a/test/adql/search/TestSimpleReplaceHandler.java b/test/adql/search/TestSimpleReplaceHandler.java
new file mode 100644
index 0000000..46bb27d
--- /dev/null
+++ b/test/adql/search/TestSimpleReplaceHandler.java
@@ -0,0 +1,67 @@
+package adql.search;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import adql.parser.ADQLParser;
+import adql.query.ADQLObject;
+import adql.query.ADQLQuery;
+import adql.query.operand.function.DefaultUDF;
+import adql.query.operand.function.MathFunction;
+
+public class TestSimpleReplaceHandler {
+
+ @Before
+ public void setUp() throws Exception{}
+
+ @Test
+ public void testReplaceRecursiveMatch(){
+ /* WHY THIS TEST?
+ *
+ * When a match item had also a match item inside it (e.g. function parameter or sub-query),
+ * both matched items (e.g. the parent and the child) must be replaced.
+ *
+ * However, if the parent is replaced first, the reference of the new parent is lost by the SimpleReplaceHandler and so,
+ * the replacement of the child will be performed on the former parent. Thus, after the whole process,
+ * in the final ADQL query, the replacement of the child won't be visible since the former parent is
+ * not referenced any more.
+ */
+
+ String testQuery = "SELECT SQRT(ABS(81)) FROM myTable";
+ try{
+ // Parse the query:
+ ADQLQuery query = (new ADQLParser()).parseQuery(testQuery);
+
+ // Check it is as expected, before the replacements:
+ assertEquals(testQuery, query.toADQL().replaceAll("\\n", " "));
+
+ // Create a replace handler:
+ SimpleReplaceHandler replaceHandler = new SimpleReplaceHandler(){
+ @Override
+ protected boolean match(ADQLObject obj){
+ return obj instanceof MathFunction;
+ }
+
+ @Override
+ protected ADQLObject getReplacer(ADQLObject objToReplace) throws UnsupportedOperationException{
+ return new DefaultUDF("foo", ((MathFunction)objToReplace).getParameters());
+ }
+ };
+
+ // Apply the replacement of all mathematical functions by a dumb UDF:
+ replaceHandler.searchAndReplace(query);
+ assertEquals(2, replaceHandler.getNbMatch());
+ assertEquals(replaceHandler.getNbMatch(), replaceHandler.getNbReplacement());
+ assertEquals("SELECT foo(foo(81)) FROM myTable", query.toADQL().replaceAll("\\n", " "));
+
+ }catch(Exception ex){
+ ex.printStackTrace(System.err);
+ fail("No error should have occured here since nothing is wrong in the ADQL query used for the test. See the stack trace in the console for more details.");
+ }
+
+ }
+
+}
--
GitLab