afs commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1914899911


##########
jena-arq/src/main/java/org/apache/jena/query/ResultSet.java:
##########
@@ -103,4 +103,8 @@ public default ResultSet materialise() {
     }
 
     public void close();
+
+    default RowSet toRowSet() {

Review Comment:
   I prefer `asRowSet` -- `as*` when it is the  same thing, different 
presentation, and `to*` when it is a created conversion.
   
   c.f. `Dataset::asDatasetGraph`



##########
jena-arq/src/main/java/org/apache/jena/sparql/engine/binding/BindingBase.java:
##########
@@ -202,4 +202,19 @@ public static int hashCode(Binding bind) {
         }
         return hash;
     }
+
+    @Override
+    public Binding detach() {
+        Binding newParent = parent == null ? null : parent.detach();
+        Binding result = newParent == parent
+                ? detachWithOriginalParent()
+                : detachWithNewParent(newParent);
+        return result;
+    }
+
+    protected Binding detachWithOriginalParent() {
+        return this;
+    }
+
+    protected abstract Binding detachWithNewParent(Binding newParent);

Review Comment:
   This would be nicer in `BindingFactory` ... if we were using Java21 and 
could use switch patterns. Oh well, another time.



##########
jena-arq/src/main/java/org/apache/jena/sparql/exec/QueryExecBuilder.java:
##########
@@ -106,4 +116,18 @@ public default boolean ask() {
             return qExec.ask();
         }
     }
+
+    /**
+     * Build and execute as a SELECT query.
+     * Creates and returns an independent in-memory table by materializing the 
underlying row set.
+     * Subsequently, {@link Table#toRowSet()} can be used to obtain a fresh 
row set view over the table.
+     */
+    public default Table table() {

Review Comment:
   See the question about "RowSet.matrialize/rewindable" above.



##########
jena-arq/src/main/java/org/apache/jena/sparql/exec/RowSet.java:
##########
@@ -93,4 +93,8 @@ public default Stream<Binding> stream() {
     public static RowSet create(QueryIterator qIter, List<Var> vars) {
         return RowSetStream.create(vars, qIter);
     }
+
+    default ResultSet toResultSet() {

Review Comment:
   ```suggestion
       default ResultSet asResultSet() {
   ```



##########
jena-arq/src/main/java/org/apache/jena/sparql/exec/QueryExec.java:
##########
@@ -54,6 +54,12 @@ public static QueryExecBuilder graph(Graph graph) {
         return QueryExecDatasetBuilder.create().graph(graph);
     }
 
+    /** Create a {@link QueryExecBuilder} initialized with an empty dataset. */
+    public static QueryExecBuilder emptyDataset() {

Review Comment:
   This is only used from `TestQueryExecution`. Please can the code go there so 
it is not in the runtime API.? 
   
   It can use`DatasetGraphFactory.empty()`.
   
   ```java
   return QueryExec.dataset(DatasetGraphFactory.empty()`);
   ```
   
   
   



##########
jena-arq/src/main/java/org/apache/jena/query/Query.java:
##########
@@ -98,7 +98,7 @@ public class Query extends Prologue implements Cloneable, 
Printable
     public static final int ORDER_UNKNOW              = -3 ;
 
     // VALUES trailing clause
-    protected TableData valuesDataBlock = null ;
+    protected TableN valuesDataBlock = null ;

Review Comment:
   It should be "immutable once built". That is, ideally, the Table interface 
should not have `addBinding`. But that's ideal.
   
   Here, the query VALUES block is syntax and fixed.
   



##########
jena-arq/src/main/java/org/apache/jena/sparql/algebra/TableFactory.java:
##########
@@ -27,31 +28,45 @@
 import org.apache.jena.sparql.algebra.table.TableUnit ;
 import org.apache.jena.sparql.core.Var ;
 import org.apache.jena.sparql.engine.QueryIterator ;
+import org.apache.jena.sparql.engine.binding.Binding;
+import org.apache.jena.sparql.exec.RowSet;
 
 public class TableFactory
 {
     public static Table createUnit()
     { return new TableUnit() ; }
-    
+
     public static Table createEmpty()
     { return new TableEmpty() ; }
 
     public static Table create()
     { return new TableN() ; }
-    
+
     public static Table create(List<Var> vars)
     { return new TableN(vars) ; }
-    
+
     public static Table create(QueryIterator queryIterator)
-    { 
+    {
         if ( queryIterator.isJoinIdentity() ) {
             queryIterator.close();
             return createUnit() ;
         }
-        
+
         return new TableN(queryIterator) ;
     }
 
     public static Table create(Var var, Node value)
     { return new Table1(var, value) ; }
+
+    /** Creates a mutable table from the detached bindings of the row set. */
+    public static Table create(RowSet rs)

Review Comment:
   I don't understand what going on. Could you explain this please? 
   
   This seems to duplicate the intent of `RowSet.materialize` and 
`RowSet.rewindable`.
   



##########
jena-arq/src/main/java/org/apache/jena/sparql/engine/binding/BindingBase.java:
##########
@@ -202,4 +202,19 @@ public static int hashCode(Binding bind) {
         }
         return hash;
     }
+
+    @Override
+    public Binding detach() {
+        Binding newParent = parent == null ? null : parent.detach();
+        Binding result = newParent == parent
+                ? detachWithOriginalParent()
+                : detachWithNewParent(newParent);

Review Comment:
   ```suggestion
           Binding newParent = (parent == null) ? null : parent.detach();
           Binding result = (newParent == parent)
                   ? detachWithOriginalParent()
                   : detachWithNewParent(newParent);
   ```



##########
jena-arq/src/test/java/org/apache/jena/sparql/graph/GraphsTests.java:
##########
@@ -106,56 +110,80 @@ protected void fillDataset(Dataset dataset) {
         m.isIsomorphicWith(calcUnion) ;
     }
 
-    @Test public void graph5() 
+    @Test public void graph5()
     {
         Dataset ds = getDataset() ;
         int x = query(queryString, 
ds.getNamedModel(Quad.defaultGraphIRI.getURI())) ;
         assertEquals(1,x) ;
     }
 
-    @Test public void graph6() 
+    @Test public void graph6()
     {
         Dataset ds = getDataset() ;
         int x = query(queryString, 
ds.getNamedModel(Quad.defaultGraphNodeGenerated.getURI())) ;
         assertEquals(1,x) ;
     }
 
-    @Test public void graph_count1() 
+    /** Test that checks that {@link QueryExecBuilder#table()} correctly 
detaches the bindings such that they remain
+     *  valid even after the query execution and the data set have been 
closed. */
+    @Test public void table1()
+    {
+        // Use a transaction if the reference data set is in one.
+        Dataset ref = getDataset() ;
+
+        Table expected = SSE.parseTable("(table (row (?s <x>) (?p <p>) (?o 
\"Default graph\") ) )") ;
+        Table actual ;
+        Dataset ds = createDataset() ;
+        try  {
+            if (ref.isInTransaction()) {
+                Txn.executeWrite(ds, () -> fillDataset(ds)) ;
+                actual = Txn.calculateRead(ds, () -> 
QueryExec.dataset(ds.asDatasetGraph()).query(queryString).table()) ;
+            } else {
+                fillDataset(ds) ;
+                actual = 
QueryExec.dataset(ds.asDatasetGraph()).query(queryString).table() ;
+            }
+        } finally {
+            ds.close() ;
+        }
+        assertEquals(expected, actual) ;

Review Comment:
   See `ResultSetCompare.exactEquals` (which should probably be in `RowSetOps`).



##########
jena-arq/src/main/java/org/apache/jena/sparql/algebra/table/TableData.java:
##########
@@ -18,23 +18,21 @@
 
 package org.apache.jena.sparql.algebra.table ;
 
+import java.util.Collections;
 import java.util.List ;
 
 import org.apache.jena.sparql.ARQException ;
 import org.apache.jena.sparql.core.Var ;
 import org.apache.jena.sparql.engine.binding.Binding ;
 
+/** Immutable table. */
 public class TableData extends TableN {
     public TableData(List<Var> variables, List<Binding> rows) {
-        super(variables, rows) ;
+        super(Collections.unmodifiableList(variables), 
Collections.unmodifiableList(rows)) ;

Review Comment:
   `Collections.unmodifiableList` only puts a wrapper around the base list. The 
base `variables` and `rows` can be modified.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscr...@jena.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscr...@jena.apache.org
For additional commands, e-mail: pr-h...@jena.apache.org

Reply via email to