Skip to content
Browse files

MODE-1680 Improved support for ORDER BY columns not in SELECT

Improved the behavior of JCR-SQL2 and JCR-QOM queries that use
columns not in the SELECT clause. Such columns are necessary
within the query processing to evaluate the ordering, but are not
to be exposed in the query results. One particular case needed
by the recent XPath improvements (to handle order-by clauses that
involve a property on the child node) resulted in the ORDER BY
column coming from a selector that was not even included in the
SELECT.

A test case recently added in other MODE-1680 commits was modified
to verify the XPath and JCR-SQL2 behavior is as expected. Also added
another query test that uses only JCR-SQL2.
  • Loading branch information...
1 parent b4198c1 commit 0f62cf771b836b93923a3c63cb0272095152aa7b @rhauch rhauch committed Mar 11, 2013
View
63 ...shape-jcr/src/main/java/org/modeshape/jcr/query/optimize/AddOrderingColumnsToSources.java
@@ -23,6 +23,7 @@
*/
package org.modeshape.jcr.query.optimize;
+import java.util.ArrayList;
import java.util.HashSet;
import java.util.LinkedList;
import java.util.List;
@@ -81,8 +82,42 @@ public void visit( ReferenceValue ref ) {
}
// Add each of the sort columns to the appropriate PROJECT nodes in the subtrees of this plan ...
+ Set<Column> columnsOnlyForSort = new HashSet<Column>();
for (Column sortColumn : sortColumns) {
- addSortColumn(context, sortNode, sortColumn);
+ if (addSortColumn(context, sortNode, sortColumn)) columnsOnlyForSort.add(sortColumn);
+ }
+
+ // If any columns were added only for the sort, we need to insert a PROJECT without the sort-only column(s)...
+ if (!columnsOnlyForSort.isEmpty()) {
+ // Find the existing project below the sort ...
+ PlanNode existingProject = sortNode.findAtOrBelow(Type.PROJECT);
+ List<Column> columns = existingProject.getPropertyAsList(Property.PROJECT_COLUMNS, Column.class);
+ List<String> types = existingProject.getPropertyAsList(Property.PROJECT_COLUMN_TYPES, String.class);
+ columns = new ArrayList<Column>(columns);
+ types = new ArrayList<String>(types);
+ for (Column sortOnlyColumn : columnsOnlyForSort) {
+ int index = columns.indexOf(sortOnlyColumn);
+ if (index >= 0) {
+ columns.remove(index);
+ types.remove(index);
+ }
+ }
+
+ // Determine the minimum selectors ...
+ Set<SelectorName> selectors = new HashSet<SelectorName>();
+ for (Column column : columns) {
+ selectors.add(column.selectorName());
+ }
+
+ // Now create a new PROJECT that wraps the SORT to remove the column(s) needed by the SORT ...
+ PlanNode newProject = new PlanNode(Type.PROJECT);
+ newProject.addSelectors(selectors);
+ newProject.setProperty(Property.PROJECT_COLUMNS, columns);
+ newProject.setProperty(Property.PROJECT_COLUMN_TYPES, types);
+
+ // And insert the new PROJECT node ...
+ sortNode.insertAsParent(newProject);
+ if (plan == sortNode) plan = newProject;
}
}
return plan;
@@ -95,24 +130,44 @@ public void visit( ReferenceValue ref ) {
* @param context the query context; may not be null
* @param node the query plan node
* @param sortColumn the column required by the sort
+ * @return true if the sort column was added, or false if it was already in the {@link Property#PROJECT_COLUMNS projected
+ * columns}
*/
- protected void addSortColumn( QueryContext context,
- PlanNode node,
- Column sortColumn ) {
+ protected boolean addSortColumn( QueryContext context,
+ PlanNode node,
+ Column sortColumn ) {
+ boolean added = false;
if (node.getSelectors().contains(sortColumn.selectorName())) {
// Get the existing projected columns ...
List<Column> columns = node.getPropertyAsList(Property.PROJECT_COLUMNS, Column.class);
List<String> types = node.getPropertyAsList(Property.PROJECT_COLUMN_TYPES, String.class);
if (columns != null && addIfMissing(context, sortColumn, columns, types)) {
node.setProperty(Property.PROJECT_COLUMNS, columns);
node.setProperty(Property.PROJECT_COLUMN_TYPES, types);
+ added = true;
}
}
// Apply recursively ...
for (PlanNode child : node) {
addSortColumn(context, child, sortColumn);
}
+
+ if (node.is(Type.SORT)) {
+ // Get the child node, which should be a PROJECT ...
+ PlanNode child = node.findAtOrBelow(Type.PROJECT);
+ if (child != null && !child.getSelectors().contains(sortColumn.selectorName())) {
+ // Make sure the PROJECT includes the missing columns, even when the selector is not included ...
+ List<Column> columns = child.getPropertyAsList(Property.PROJECT_COLUMNS, Column.class);
+ List<String> types = child.getPropertyAsList(Property.PROJECT_COLUMN_TYPES, String.class);
+ if (columns != null && addIfMissing(context, sortColumn, columns, types)) {
+ child.setProperty(Property.PROJECT_COLUMNS, columns);
+ child.setProperty(Property.PROJECT_COLUMN_TYPES, types);
+ added = true;
+ }
+ }
+ }
+ return added;
}
/**
View
18 modeshape-jcr/src/main/java/org/modeshape/jcr/query/process/QueryProcessor.java
@@ -320,22 +320,6 @@ protected ProcessingComponent createComponent( QueryCommand originalQuery,
case SORT:
// Create the component under the SORT ...
assert node.getChildCount() == 1;
-
- List<PlanNode> project = node.findAllAtOrBelow(Type.PROJECT);
- assert project != null;
-
- ArrayList<Column> allColumns = new ArrayList();
- for (PlanNode p : project) {
- allColumns.addAll(p.getPropertyAsList(Property.PROJECT_COLUMNS, Column.class));
- }
-
- ArrayList<String> ctypes = new ArrayList();
- for (PlanNode p : project) {
- ctypes.addAll(p.getPropertyAsList(Property.PROJECT_COLUMN_TYPES, String.class));
- }
-
- QueryResultColumns orderColumns = new QueryResultColumns(allColumns, ctypes, context.getHints().hasFullTextSearch);
-
ProcessingComponent sortDelegate = createComponent(originalQuery,
context,
node.getFirstChild(),
@@ -359,7 +343,7 @@ protected ProcessingComponent createComponent( QueryCommand originalQuery,
if (alias != null) sourceNamesByAlias.put(alias, name);
}
// Now create the sorting component ...
- component = new SortValuesComponent(sortDelegate, orderings, orderColumns, sourceNamesByAlias);
+ component = new SortValuesComponent(sortDelegate, orderings, sourceNamesByAlias);
} else {
// Order by the location(s) because it's before a merge-join ...
component = new SortLocationsComponent(sortDelegate);
View
26 modeshape-jcr/src/main/java/org/modeshape/jcr/query/process/QueryResultColumns.java
@@ -78,6 +78,7 @@ public static QueryResultColumns empty() {
private List<String> tupleValueNames;
private final Map<String, String> selectorNameByColumnName;
private final Map<String, Integer> columnIndexByColumnName;
+ private final Map<String, String> columnTypeNameByColumnName;
private final Map<String, Integer> locationIndexBySelectorName;
private final Map<String, Integer> locationIndexByColumnName;
private final Map<Integer, Integer> locationIndexByColumnIndex;
@@ -126,6 +127,7 @@ protected QueryResultColumns( boolean includeFullTextSearchScores,
this.columns = columns != null ? Collections.<Column>unmodifiableList(columns) : NO_COLUMNS;
this.columnTypes = columnTypes != null ? Collections.<String>unmodifiableList(columnTypes) : NO_TYPES;
this.columnIndexByColumnName = new HashMap<String, Integer>();
+ this.columnTypeNameByColumnName = new HashMap<String, String>();
Set<String> selectors = new HashSet<String>();
final int columnCount = this.columns.size();
this.locationIndexBySelectorName = new HashMap<String, Integer>();
@@ -167,6 +169,7 @@ protected QueryResultColumns( boolean includeFullTextSearchScores,
propertyNameByColumnName.put(columnName, column.getPropertyName());
selectorNameByColumnName.put(columnName, selectorName);
columnIndexByColumnName.put(columnName, new Integer(i));
+ columnTypeNameByColumnName.put(columnName, this.columnTypes.get(i));
locationIndexByColumnIndex.put(new Integer(i), selectorIndex);
locationIndexByColumnName.put(columnName, selectorIndex);
// Insert the entry by selector name and property name ...
@@ -204,6 +207,7 @@ private QueryResultColumns( List<Column> columns,
assert columns != null;
this.columns = Collections.unmodifiableList(columns);
this.columnIndexByColumnName = new HashMap<String, Integer>();
+ this.columnTypeNameByColumnName = new HashMap<String, String>();
this.locationIndexBySelectorName = new HashMap<String, Integer>();
this.locationIndexByColumnIndex = new HashMap<Integer, Integer>();
this.locationIndexByColumnName = new HashMap<String, Integer>();
@@ -248,21 +252,21 @@ private QueryResultColumns( List<Column> columns,
}
// if column index is still null, lookup the column index by property name.
if (columnIndex == null) {
- columnNameWithoutSelector = column.getPropertyName();
- if (columnNameWithoutSelector.startsWith(selectorName + ".")
- && columnNameWithoutSelector.length() > (selectorName.length() + 1)) {
- columnNameWithoutSelector = columnNameWithoutSelector.substring(selectorName.length() + 1);
- }
- columnIndex = wrappedAround.columnIndexForName(columnNameWithoutSelector);
- if (columnIndex == null) {
- String columnNameWithSelector = column.selectorName() + "." + columnNameWithoutSelector;
- columnIndex = wrappedAround.columnIndexForName(columnNameWithSelector);
- }
+ columnNameWithoutSelector = column.getPropertyName();
+ if (columnNameWithoutSelector.startsWith(selectorName + ".")
+ && columnNameWithoutSelector.length() > (selectorName.length() + 1)) {
+ columnNameWithoutSelector = columnNameWithoutSelector.substring(selectorName.length() + 1);
+ }
+ columnIndex = wrappedAround.columnIndexForName(columnNameWithoutSelector);
+ if (columnIndex == null) {
+ String columnNameWithSelector = column.selectorName() + "." + columnNameWithoutSelector;
+ columnIndex = wrappedAround.columnIndexForName(columnNameWithSelector);
+ }
}
}
assert columnIndex != null;
columnIndexByColumnName.put(columnName, columnIndex);
- String columnType = wrappedAround.getColumnTypes().get(columnIndex.intValue());
+ String columnType = wrappedAround.columnTypeNameByColumnName.get(columnName);
types.add(columnType);
Integer selectorIndex = new Integer(wrappedAround.getLocationIndex(selectorName));
locationIndexBySelectorName.put(selectorName, selectorIndex);
View
9 modeshape-jcr/src/main/java/org/modeshape/jcr/query/process/SortValuesComponent.java
@@ -47,19 +47,12 @@
private final Comparator<Object[]> sortingComparator;
public SortValuesComponent( ProcessingComponent delegate,
- List<Ordering> orderings,
+ List<Ordering> orderings,
Map<SelectorName, SelectorName> sourceNamesByAlias ) {
super(delegate);
this.sortingComparator = createSortComparator(delegate.getContext(), delegate.getColumns(), orderings, sourceNamesByAlias);
}
- public SortValuesComponent( ProcessingComponent delegate,
- List<Ordering> orderings, QueryResultColumns columns,
- Map<SelectorName, SelectorName> sourceNamesByAlias ) {
- super(delegate);
- this.sortingComparator = createSortComparator(delegate.getContext(), columns, orderings, sourceNamesByAlias);
- }
-
/**
* @return sortingComparator
*/
View
85 modeshape-jcr/src/test/java/org/modeshape/jcr/JcrQueryManagerTest.java
@@ -40,7 +40,6 @@
import java.io.File;
import java.io.IOException;
import java.io.InputStream;
-import java.math.BigDecimal;
import java.net.URI;
import java.net.URISyntaxException;
import java.net.URL;
@@ -349,7 +348,7 @@ protected void assertResults( Query query,
if (print /*|| result.getNodes().getSize() != numberOfResults || result.getRows().getSize() != numberOfResults*/) {
System.out.println();
System.out.println(query);
- System.out.println(" plan -> " + ((JcrQueryResult)result).getPlan());
+ System.out.println(" plan -> " + ((org.modeshape.jcr.api.query.QueryResult)result).getPlan());
System.out.println(result);
}
if (result.getSelectorNames().length == 1) {
@@ -622,7 +621,7 @@ public void shouldBeAbleToCreateAndExecuteJcrSql2QueryWithOrderByUsingColumnNotI
@FixFor( "MODE-1095" )
@Test
- public void shouldBeAbleToCreateAndExecuteJcrSql2QueryWithJoinCriteriaOnColumnsNotInSelect() throws RepositoryException {
+ public void shouldBeAbleToCreateAndExecuteJcrSql2QueryWithJoinCriteriaOnColumnsInSelect() throws RepositoryException {
String sql = "SELECT x.*, y.* FROM [nt:unstructured] AS x INNER JOIN [nt:unstructured] AS y ON x.somethingElse = y.propC ORDER BY x.propC";
Query query = session.getWorkspace().getQueryManager().createQuery(sql, Query.JCR_SQL2);
assertThat(query, is(notNullValue()));
@@ -636,6 +635,21 @@ public void shouldBeAbleToCreateAndExecuteJcrSql2QueryWithJoinCriteriaOnColumnsN
assertThat(row1.getNode("y").getPath(), is("/Other/NodeA[2]"));
}
+ @FixFor( {"MODE-1095", "MODE-1680"} )
+ @Test
+ public void shouldBeAbleToCreateAndExecuteJcrSql2QueryWithJoinCriteriaOnColumnsNotInSelect() throws RepositoryException {
+ String sql = "SELECT y.* FROM [nt:unstructured] AS x INNER JOIN [nt:unstructured] AS y ON x.somethingElse = y.propC ORDER BY x.propC";
+ Query query = session.getWorkspace().getQueryManager().createQuery(sql, Query.JCR_SQL2);
+ assertThat(query, is(notNullValue()));
+ QueryResult result = query.execute();
+ assertThat(result, is(notNullValue()));
+ assertResults(query, result, 1);
+ assertResultsHaveColumns(result, allOf(allColumnNames("y")));
+ RowIterator rows = result.getRows();
+ Row row1 = rows.nextRow();
+ assertThat(row1.getNode().getPath(), is("/Other/NodeA[2]"));
+ }
+
@FixFor( "MODE-1055" )
@Test
public void shouldBeAbleToCreateAndExecuteJcrSql2QueryToFindAllNodesWithCriteria() throws RepositoryException {
@@ -993,14 +1007,15 @@ public void shouldBeAbleToExecuteQueryForAllColumns() throws RepositoryException
assertResults(query, result, 9L);
}
- @FixFor ( "MODE-1833" )
+ @FixFor( "MODE-1833" )
@Test
public void shouldBeAbleToQueryAllColumnsOnSimpleType() throws RepositoryException {
QueryManager queryManager = session.getWorkspace().getQueryManager();
QueryObjectModelFactory factory = queryManager.getQOMFactory();
Query query = factory.createQuery(factory.selector("modetest:simpleType", "type1"),
- null, null,
- new Column[]{factory.column("type1", null, null)});
+ null,
+ null,
+ new Column[] {factory.column("type1", null, null)});
assertThat(query, is(notNullValue()));
QueryResult result = query.execute();
assertThat(result, is(notNullValue()));
@@ -2973,39 +2988,42 @@ public void shouldFindChildrenOfRootUsingIsChildNodeCriteria() throws Exception
assertNodesAreFound(queryString, Query.JCR_SQL2, "/jcr:system", "/Cars", "/Other", "/NodeB", "/node1", "/node2");
}
+ @SuppressWarnings( "deprecation" )
+ @FixFor( "MODE-1680" )
@Test
public void testOrderByWithAliases() throws Exception {
- //fill the repository with test data
+ // fill the repository with test data
Node src = session.getRootNode().addNode("src", "nt:folder");
- //add node f1 with child jcr:content
+ // add node f1 with child jcr:content
Node f1 = src.addNode("f1", "nt:file");
f1.addMixin("mix:simpleVersionable");
Node content1 = f1.addNode("jcr:content", "nt:resource");
content1.setProperty("jcr:data", session.getValueFactory().createBinary("Node f1".getBytes()));
- //save and slip a bit to have difference in time of node creation.
+ // save and slip a bit to have difference in time of node creation.
session.save();
- Thread.currentThread().sleep(1000);
-
- //add node f2 with child jcr:content
+ Thread.sleep(1000);
+
+ // add node f2 with child jcr:content
Node f2 = src.addNode("f2", "nt:file");
f2.addMixin("mix:simpleVersionable");
Node content2 = f2.addNode("jcr:content", "nt:resource");
content2.setProperty("jcr:data", session.getValueFactory().createBinary("Node f2".getBytes()));
session.save();
-
- System.out.println("-------------------- MyQueryTest---------------------");
- String descOrder = "SELECT [nt:file].[jcr:created] FROM [nt:file] INNER JOIN [nt:base] AS content ON ISCHILDNODE(content,[nt:file]) WHERE ([nt:file].[jcr:mixinTypes] = 'mix:simpleVersionable' AND NAME([nt:file]) LIKE 'f%') ORDER BY content.[jcr:created] DESC";
- String ascOrder = "SELECT [nt:file].[jcr:created] FROM [nt:file] INNER JOIN [nt:base] AS content ON ISCHILDNODE(content,[nt:file]) WHERE ([nt:file].[jcr:mixinTypes] = 'mix:simpleVersionable' AND NAME([nt:file]) LIKE 'f%') ORDER BY content.[jcr:created] ASC";
+ // print = true;
+ printMessage("-------------------- MyQueryTest---------------------");
+
+ String descOrder = "SELECT [nt:file].[jcr:created] FROM [nt:file] INNER JOIN [nt:base] AS content ON ISCHILDNODE(content,[nt:file]) WHERE ([nt:file].[jcr:mixinTypes] = 'mix:simpleVersionable' AND NAME([nt:file]) LIKE 'f%') ORDER BY content.[jcr:lastModified] DESC";
+ String ascOrder = "SELECT [nt:file].[jcr:created] FROM [nt:file] INNER JOIN [nt:base] AS content ON ISCHILDNODE(content,[nt:file]) WHERE ([nt:file].[jcr:mixinTypes] = 'mix:simpleVersionable' AND NAME([nt:file]) LIKE 'f%') ORDER BY content.[jcr:lastModified] ASC";
QueryManager queryManager = session.getWorkspace().getQueryManager();
Query query = queryManager.createQuery(descOrder, Query.JCR_SQL2);
QueryResult result = query.execute();
- //checking first query
+ // checking first query
RowIterator it = result.getRows();
assertEquals(2, it.getSize());
@@ -3015,11 +3033,40 @@ public void testOrderByWithAliases() throws Exception {
assertEquals("f2", n1.getName());
assertEquals("f1", n2.getName());
- //the same request with other order
+ // the same request with other order
query = queryManager.createQuery(ascOrder, Query.JCR_SQL2);
result = query.execute();
- //checking second query
+ // checking second query
+ it = result.getRows();
+ assertEquals(2, it.getSize());
+
+ n1 = it.nextRow().getNode();
+ n2 = it.nextRow().getNode();
+
+ assertEquals("f1", n1.getName());
+ assertEquals("f2", n2.getName());
+
+ // Try the XPath query ...
+ String descOrderX = "/jcr:root//element(*,nt:file)[(@jcr:mixinTypes = 'mix:simpleVersionable')] order by jcr:content/@jcr:lastModified descending";
+ String ascOrderX = "/jcr:root//element(*,nt:file)[(@jcr:mixinTypes = 'mix:simpleVersionable')] order by jcr:content/@jcr:lastModified ascending";
+ query = queryManager.createQuery(descOrderX, Query.XPATH);
+ result = query.execute();
+ // checking first query
+ it = result.getRows();
+ assertEquals(2, it.getSize());
+
+ n1 = it.nextRow().getNode();
+ n2 = it.nextRow().getNode();
+
+ assertEquals("f2", n1.getName());
+ assertEquals("f1", n2.getName());
+
+ // the same request with other order
+ query = queryManager.createQuery(ascOrderX, Query.XPATH);
+ result = query.execute();
+
+ // checking second query
it = result.getRows();
assertEquals(2, it.getSize());

0 comments on commit 0f62cf7

Please sign in to comment.
Something went wrong with that request. Please try again.