Commit ad2acca3 authored by gmantele's avatar gmantele
Browse files

[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".
parent b8138111
Loading
Loading
Loading
Loading
+64 −12
Original line number Diff line number Diff line
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&eacute;gory Mantelet (CDS)
 * @version 06/2011
 * @author Gr&eacute;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;
		}
	}

	/**
+67 −0
Original line number Diff line number Diff line
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.");
		}

	}

}