Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
afs merged PR #2925: URL: https://github.com/apache/jena/pull/2925 -- 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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on PR #2925: URL: https://github.com/apache/jena/pull/2925#issuecomment-2629363936 > I'd mostly finished reviewing - does the TableBuilder make a difference? It's not a big difference - It exists now because you suggested that this might be the better approach - and I used it in the fixed code. Other than that its finished. -- 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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
afs commented on PR #2925: URL: https://github.com/apache/jena/pull/2925#issuecomment-2629360540 I'd mostly finished reviewing - does the TableBuilder make a difference? Let me know when this is ready. The main issues are to do with the formal description of LATERAL, and not the core in this PR which is faithful to it the formal description. A deeper reworking of execution could so better but it would also change other places. Functionality before optimization! -- 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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on PR #2925: URL: https://github.com/apache/jena/pull/2925#issuecomment-2629058496 I added a TableBuilder class for building immutable tables. In accordance with the discussion, in `QueryIterLateral` I removed the algebra compatibility check and updated the table creation to use the new TableBuilder. Migrating existing table creation would be a future endeavor. -- 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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1938320408
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -291,18 +291,25 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
-// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+
+TableBuilder tableBuilder = TableFactory.builder();
Review Comment:
Based on the discussion, I now added a TableBuilder class for building
immutable Tables.
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
afs commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1930805943
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +295,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
+commonVars.add(v);
+} else {
+vars.add(v);
+}
+});
+
+List bindings = new ArrayList<>(table.size());
BindingBuilder builder = BindingFactory.builder();
table.iterator(null).forEachRemaining(row->{
-builder.reset();
-builder.addAll(row);
-builder.addAll(binding);
-table2.addBinding(builder.build());
+if (Algebra.compatible(row, binding, commonVars.iterator())) {
Review Comment:
> For the sake of closing the issue, should I modify/simplify the logic
Yes, please.
> Well, the tradeoff to what extend existing implementations need to drive
restrictions of new features in a standard.
SHACL WG has started and SHACL Values Insertion (which is similar but
different) will need SPARQL syntax level solution, which implies static rules
for scoping. As will parameterized query.
I hope, in spirit, these different situations have a related approach.
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on PR #2925: URL: https://github.com/apache/jena/pull/2925#issuecomment-2599753942 Just to summarize: The main open point for this PR is whether to remove the `Algebra.compatible` check on the `commonVars` under the assumption that the bindings have to be compatible anyway (because of `SyntaxVarScope.checkLATERAL`). -- 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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1918738192
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +295,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
+commonVars.add(v);
+} else {
+vars.add(v);
+}
+});
+
+List bindings = new ArrayList<>(table.size());
BindingBuilder builder = BindingFactory.builder();
table.iterator(null).forEachRemaining(row->{
-builder.reset();
-builder.addAll(row);
-builder.addAll(binding);
-table2.addBinding(builder.build());
+if (Algebra.compatible(row, binding, commonVars.iterator())) {
Review Comment:
> Sorry, I have to get Jena 5.3.0 out without this.
No worries, whenever it's ready.
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1918597769
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +295,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
+commonVars.add(v);
+} else {
+vars.add(v);
+}
+});
+
+List bindings = new ArrayList<>(table.size());
BindingBuilder builder = BindingFactory.builder();
table.iterator(null).forEachRemaining(row->{
-builder.reset();
-builder.addAll(row);
-builder.addAll(binding);
-table2.addBinding(builder.build());
+if (Algebra.compatible(row, binding, commonVars.iterator())) {
Review Comment:
> In general, some system have optimizations that on the static analysis of
the query.
Well, the tradeoff to what extend existing implementations need to drive
restrictions of new features in a standard.
For the sake of closing the issue, should I modify/simplify the logic in
QueryIterLateral or should I leave the slightly more general approach as it is
for now? ~Essentially, the var scope check should prevent non-empty sets of
commonVars.~ No, commonVars is non-empty in the case of nested laterals -
because the outer lateral already injects its binding into the unit tables - so
subsequent inner laterals attempt to do the same injection. Perhaps the way it
is is safe - the alternative is to remove the compatibility check on commonVars
(and the commonVars set itself) - because the injections should be compatible
in the first place.
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1918597769
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +295,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
+commonVars.add(v);
+} else {
+vars.add(v);
+}
+});
+
+List bindings = new ArrayList<>(table.size());
BindingBuilder builder = BindingFactory.builder();
table.iterator(null).forEachRemaining(row->{
-builder.reset();
-builder.addAll(row);
-builder.addAll(binding);
-table2.addBinding(builder.build());
+if (Algebra.compatible(row, binding, commonVars.iterator())) {
Review Comment:
> In general, some system have optimizations that on the static analysis of
the query.
Well, the tradeoff to what extend existing implementations need to drive
restrictions of new features in a standard.
For the sake of closing the issue, should I modify/simplify the logic in
QueryIterLateral or should I leave the slightly more general approach as it is
for now? ~Essentially, the var scope check should prevent non-empty sets of
commonVars.~ No, commonVars is non-empty in the case of nested laterals -
because the outer lateral already injects its binding into the unit tables - so
subsequent inner laterals attempt to do the same injection. Perhaps the way it
is is safe - the alternative is to remove the compatibility check on commonVars
(and the commonVars set itself).
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1918597769
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +295,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
+commonVars.add(v);
+} else {
+vars.add(v);
+}
+});
+
+List bindings = new ArrayList<>(table.size());
BindingBuilder builder = BindingFactory.builder();
table.iterator(null).forEachRemaining(row->{
-builder.reset();
-builder.addAll(row);
-builder.addAll(binding);
-table2.addBinding(builder.build());
+if (Algebra.compatible(row, binding, commonVars.iterator())) {
Review Comment:
> In general, some system have optimizations that on the static analysis of
the query.
Well, the tradeoff to what extend existing implementations need to drive
restrictions of new features in a standard.
For the sake of closing the issue, should I modify/simplify the logic in
QueryIterLateral or should I leave the slightly more general approach as it is
for now? ~Essentially, the var scope check should prevent non-empty sets of
commonVars.~ No, commonVars is non-empty in the case of nested laterals -
because the outer lateral already injects its binding into the unit tables - so
subsequent inner laterals attempt to do the same injection. Perhaps it's safe
the way is?
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1918597769
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +295,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
+commonVars.add(v);
+} else {
+vars.add(v);
+}
+});
+
+List bindings = new ArrayList<>(table.size());
BindingBuilder builder = BindingFactory.builder();
table.iterator(null).forEachRemaining(row->{
-builder.reset();
-builder.addAll(row);
-builder.addAll(binding);
-table2.addBinding(builder.build());
+if (Algebra.compatible(row, binding, commonVars.iterator())) {
Review Comment:
> In general, some system have optimizations that on the static analysis of
the query.
Well, the tradeoff to what extend existing implementations need to drive
restrictions of new features in a standard.
For the sake of closing the issue, should I modify/simplify the logic in
QueryIterLateral or should I leave the slightly more general approach as it is
for now? ~Essentially, the var scope check should prevent non-empty sets of
commonVars.~ No, commonVars is non-empty in the case of nested laterals -
because the outer lateral already injects its binding into the unit tables - so
subsequent inner laterals attempt to do the same injection. Perhaps its safe
the way is?
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1918597769
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +295,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
+commonVars.add(v);
+} else {
+vars.add(v);
+}
+});
+
+List bindings = new ArrayList<>(table.size());
BindingBuilder builder = BindingFactory.builder();
table.iterator(null).forEachRemaining(row->{
-builder.reset();
-builder.addAll(row);
-builder.addAll(binding);
-table2.addBinding(builder.build());
+if (Algebra.compatible(row, binding, commonVars.iterator())) {
Review Comment:
> In general, some system have optimizations that on the static analysis of
the query.
Well, the tradeoff to what extend existing implementations need to drive
restrictions of new features in a standard.
For the sake of closing the issue, should I modify/simplify the logic in
QueryIterLateral or should I leave the slightly more general approach as it is
for now? ~Essentially, the var scope check should prevent non-empty sets of
commonVars.~ No, commonVars is non-empty in the case of nested laterals. I need
to check what change would be necessary.
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1918597769
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +295,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
+commonVars.add(v);
+} else {
+vars.add(v);
+}
+});
+
+List bindings = new ArrayList<>(table.size());
BindingBuilder builder = BindingFactory.builder();
table.iterator(null).forEachRemaining(row->{
-builder.reset();
-builder.addAll(row);
-builder.addAll(binding);
-table2.addBinding(builder.build());
+if (Algebra.compatible(row, binding, commonVars.iterator())) {
Review Comment:
> In general, some system have optimizations that on the static analysis of
the query.
Well, the tradeoff to what extend existing implementations need to drive
restrictions of new features in a standard.
For the sake of closing the issue, should I modify/simplify the logic in
QueryIterLateral or should I leave the slightly more general approach as it is
for now? Essentially, the var scope check should prevent non-empty sets of
commonVars.
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +295,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
+commonVars.add(v);
+} else {
+vars.add(v);
+}
+});
+
+List bindings = new ArrayList<>(table.size());
BindingBuilder builder = BindingFactory.builder();
table.iterator(null).forEachRemaining(row->{
-builder.reset();
-builder.addAll(row);
-builder.addAll(binding);
-table2.addBinding(builder.build());
+if (Algebra.compatible(row, binding, commonVars.iterator())) {
Review Comment:
> In general, some system have optimizations that on the static analysis of
the query.
Well, the tradeoff to what extend existing implementations need to drive
restrictions of new features in a standard.
For the sake of closing the issue, should I modify/simplify the logic in
QueryIterLateral or should I leave the slightly more general approach as it is
for now? Essentially, the var scope check should prevent non-empty sets of
commonVars.
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
afs commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1918588438
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +295,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
+commonVars.add(v);
+} else {
+vars.add(v);
+}
+});
+
+List bindings = new ArrayList<>(table.size());
BindingBuilder builder = BindingFactory.builder();
table.iterator(null).forEachRemaining(row->{
-builder.reset();
-builder.addAll(row);
-builder.addAll(binding);
-table2.addBinding(builder.build());
+if (Algebra.compatible(row, binding, commonVars.iterator())) {
Review Comment:
Sorry, I have to get Jena 5.3.0 out without this.
This issue is important and something that is, I hope, going to get more
importance in the SPARQL community.
[WG sparql-query PR 177](https://github.com/w3c/sparql-query/pull/177) at
least gets the essential machinery into the resp with the WG contribution
licnesing. A small step, but a step.
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
afs commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1918581831
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +295,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
+commonVars.add(v);
+} else {
+vars.add(v);
+}
+});
+
+List bindings = new ArrayList<>(table.size());
BindingBuilder builder = BindingFactory.builder();
table.iterator(null).forEachRemaining(row->{
-builder.reset();
-builder.addAll(row);
-builder.addAll(binding);
-table2.addBinding(builder.build());
+if (Algebra.compatible(row, binding, commonVars.iterator())) {
Review Comment:
> Hm, depending on the viewpoint it is the effective query _after_
substitution,
My bad - I was thinking about `GROUP BY`.
For `ORDER BY`, the query is illegal by the assignment restriction.
> To me it looks like it would only retain bindings where ?o = 123.
That would turn `BIND` into a filter. (ARQ's extension `LET` does that.)
We have to have a restriction of some kind. The SEP-0007 design makes it a
compile-time condition (test once per query).
In general, some system have optimizations that on the static analysis of
the query. Making the RHS of a LATERAL have runtime variance would interfere.
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1918441638
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +295,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
+commonVars.add(v);
+} else {
+vars.add(v);
+}
+});
+
+List bindings = new ArrayList<>(table.size());
BindingBuilder builder = BindingFactory.builder();
table.iterator(null).forEachRemaining(row->{
-builder.reset();
-builder.addAll(row);
-builder.addAll(binding);
-table2.addBinding(builder.build());
+if (Algebra.compatible(row, binding, commonVars.iterator())) {
Review Comment:
> `SELECT *` is effectively `SELECT ?employee` which works.
Hm, depending on the viewpoint it is the effective query *after*
substitution, but before that, it's `SELECT ?department ?employee` as to make
`?department` in-scope such that the substitution could affect the table and
discard its incompatible rows.
I think the scope error is in-line with the SEP7 statement:
> "Disallow syntactic forms that set variables that may already be present
in the current row."
But I think that this is too restrictive, because it prevents several useful
cases for inlining data using VALUES blocks. IMO if substitution leads to
incompatible bindings then they should just be discarded as usual - no? Perhaps
you have corner cases in mind where under this logic the substitution could
become ambiguous and that this might be the reason for the restrictions.
Removing the restrictions on `Element_Bind` and `Element_Data` in
`checkLATERAL` in order to make the above "department-employee" query work
causes the following test cases to fail because they no longer raise an
varscope/syntax error:
syntax-lateral-bad-{[04](https://github.com/apache/jena/blob/main/jena-arq/testing/ARQ/Syntax/Syntax-Lateral/syntax-lateral-bad-04),
[05](https://github.com/apache/jena/blob/main/jena-arq/testing/ARQ/Syntax/Syntax-Lateral/syntax-lateral-bad-05),
[06](https://github.com/apache/jena/blob/main/jena-arq/testing/ARQ/Syntax/Syntax-Lateral/syntax-lateral-bad-06),
[08](https://github.com/apache/jena/blob/main/jena-arq/testing/ARQ/Syntax/Syntax-Lateral/syntax-lateral-bad-08)}.arq
For example, `syntax-lateral-bad-04`:
```sparql
SELECT * {
?s ?p ?o
LATERAL { BIND( 123 AS ?o) }
}
```
This is certainly not an efficient way to write a query, but I don't
understand why the query should be syntactically illegal in the first place. To
me it looks like it would only retain bindings where ?o = 123. So it would be
the task of an optimizer to rewrite the `Lateral/Bind` to a plain `FILTER(?o =
123)`.
When disabling the SyntaxVarScope check for ElementBind, each ?o gets
injected into the unit table beneath `(extend ((?o 123)) (table unit))` such
as `(extend ((?o 123)) (table (row (?o 456` and the evaluation of OpExtend
discards the incompatible binding - which seems like the natural way it should
be to me.
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1918441638
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +295,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
+commonVars.add(v);
+} else {
+vars.add(v);
+}
+});
+
+List bindings = new ArrayList<>(table.size());
BindingBuilder builder = BindingFactory.builder();
table.iterator(null).forEachRemaining(row->{
-builder.reset();
-builder.addAll(row);
-builder.addAll(binding);
-table2.addBinding(builder.build());
+if (Algebra.compatible(row, binding, commonVars.iterator())) {
Review Comment:
> `SELECT *` is effectively `SELECT ?employee` which works.
Hm, depending on the viewpoint it is the effective query *after*
substitution, but before that, it's `SELECT ?department ?employee` as to make
`?department` in-scope such that the substitution could affect the table and
discard its incompatible rows.
I think the scope error is in-line with the SEP7 statement:
> "Disallow syntactic forms that set variables that may already be present
in the current row."
But I think that this is too restrictive, because it prevents several useful
cases for inlining data using VALUES blocks. IMO if substitution leads to
incompatible bindings then they should just be discarded as usual - no? Perhaps
you have corner cases in mind where under this logic the substitution could
become ambiguous and that this might be the reason for the restrictions.
Removing the restrictions on `Element_Bind` and `Element_Data` in
`checkLATERAL` in order to make the above "department-employee" query work
causes the following test cases to fail because they no longer raise an syntax
error:
syntax-lateral-bad-{[04](https://github.com/apache/jena/blob/main/jena-arq/testing/ARQ/Syntax/Syntax-Lateral/syntax-lateral-bad-04),
[05](https://github.com/apache/jena/blob/main/jena-arq/testing/ARQ/Syntax/Syntax-Lateral/syntax-lateral-bad-05),
[06](https://github.com/apache/jena/blob/main/jena-arq/testing/ARQ/Syntax/Syntax-Lateral/syntax-lateral-bad-06),
[08](https://github.com/apache/jena/blob/main/jena-arq/testing/ARQ/Syntax/Syntax-Lateral/syntax-lateral-bad-08)}.arq
For example, `syntax-lateral-bad-04`:
```sparql
SELECT * {
?s ?p ?o
LATERAL { BIND( 123 AS ?o) }
}
```
This is certainly not an efficient way to write a query, but I don't
understand why the query should be syntactically illegal in the first place. To
me it looks like it would only retain bindings where ?o = 123. So it would be
the task of an optimizer to rewrite the `Lateral/Bind` to a plain `FILTER(?o =
123)`.
When disabling the SyntaxVarScope check for ElementBind, each ?o gets
injected into the unit table beneath `(extend ((?o 123)) (table unit))` such
as `(extend ((?o 123)) (table (row (?o 456` and the evaluation of OpExtend
discards the incompatible binding - which seems like the natural way it should
be to me.
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1918441638
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +295,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
+commonVars.add(v);
+} else {
+vars.add(v);
+}
+});
+
+List bindings = new ArrayList<>(table.size());
BindingBuilder builder = BindingFactory.builder();
table.iterator(null).forEachRemaining(row->{
-builder.reset();
-builder.addAll(row);
-builder.addAll(binding);
-table2.addBinding(builder.build());
+if (Algebra.compatible(row, binding, commonVars.iterator())) {
Review Comment:
> `SELECT *` is effectively `SELECT ?employee` which works.
Hm, depending on the viewpoint it is the effective query *after*
substitution, but before that, it's `SELECT ?department ?employee` as to make
`?department` in-scope such that the substitution could affect the table and
discard its incompatible rows.
I think the scope error is in-line with the SEP7 statement:
> "Disallow syntactic forms that set variables that may already be present
in the current row."
But I think that this is too restrictive, because it prevents several useful
cases for inlining data using VALUES blocks. IMO if substitution leads to
incompatible bindings then they should just be discarded as usual - no? Perhaps
you have corner cases in mind where under this logic the substitution could
become ambiguous and that this might be the reason for the restrictions.
Removing the restrictions on `Element_Bind` and `Element_Data` in
`checkLATERAL` in order to make the above "department-employee" query work
causes the following test cases to fail:
syntax-lateral-bad-{[04](https://github.com/apache/jena/blob/main/jena-arq/testing/ARQ/Syntax/Syntax-Lateral/syntax-lateral-bad-04),
[05](https://github.com/apache/jena/blob/main/jena-arq/testing/ARQ/Syntax/Syntax-Lateral/syntax-lateral-bad-05),
[06](https://github.com/apache/jena/blob/main/jena-arq/testing/ARQ/Syntax/Syntax-Lateral/syntax-lateral-bad-06),
[08](https://github.com/apache/jena/blob/main/jena-arq/testing/ARQ/Syntax/Syntax-Lateral/syntax-lateral-bad-08)}.arq
For example, `syntax-lateral-bad-04`:
```sparql
SELECT * {
?s ?p ?o
LATERAL { BIND( 123 AS ?o) }
}
```
This is certainly not an efficient way to write a query, but I don't
understand why the query should be syntactically illegal in the first place. To
me it looks like it would only retain bindings where ?o = 123. So it would be
the task of an optimizer to rewrite the `Lateral/Bind` to a plain `FILTER(?o =
123)`.
When disabling the SyntaxVarScope check for ElementBind, each ?o gets
injected into the unit table beneath `(extend ((?o 123)) (table unit))` such
as `(extend ((?o 123)) (table (row (?o 456` and the evaluation of OpExtend
discards the incompatible binding - which seems like the natural way it should
be to me.
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1918441638
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +295,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
+commonVars.add(v);
+} else {
+vars.add(v);
+}
+});
+
+List bindings = new ArrayList<>(table.size());
BindingBuilder builder = BindingFactory.builder();
table.iterator(null).forEachRemaining(row->{
-builder.reset();
-builder.addAll(row);
-builder.addAll(binding);
-table2.addBinding(builder.build());
+if (Algebra.compatible(row, binding, commonVars.iterator())) {
Review Comment:
> `SELECT *` is effectively `SELECT ?employee` which works.
Hm, depending on the viewpoint it is the effective query *after*
substitution, but before that, its `SELECT ?department ?employee` as to make
`?department` in-scope such that the substitution could affect the table and
discard its incompatible rows.
I think the scope error is in-line with the SEP7 statement:
> "Disallow syntactic forms that set variables that may already be present
in the current row."
But I think that this is too restrictive, because it prevents several useful
cases for inlining data using VALUES blocks. IMO if substitution leads to
incompatible bindings then they should just be discarded as usual - no? Perhaps
you have corner cases in mind where under this logic the substitution could
become ambiguous and that this might be the reason for the restrictions.
Removing the restrictions on `Element_Bind` and `Element_Data` in
`checkLATERAL` in order to make the above "department-employee" query work
causes the following test cases to fail:
syntax-lateral-bad-{[04](https://github.com/apache/jena/blob/main/jena-arq/testing/ARQ/Syntax/Syntax-Lateral/syntax-lateral-bad-04),
[05](https://github.com/apache/jena/blob/main/jena-arq/testing/ARQ/Syntax/Syntax-Lateral/syntax-lateral-bad-05),
[06](https://github.com/apache/jena/blob/main/jena-arq/testing/ARQ/Syntax/Syntax-Lateral/syntax-lateral-bad-06),
[08](https://github.com/apache/jena/blob/main/jena-arq/testing/ARQ/Syntax/Syntax-Lateral/syntax-lateral-bad-08)}.arq
For example, `syntax-lateral-bad-04`:
```sparql
SELECT * {
?s ?p ?o
LATERAL { BIND( 123 AS ?o) }
}
```
This is certainly not an efficient way to write a query, but I don't
understand why the query should be syntactically illegal in the first place. To
me it looks like it would only retain bindings where ?o = 123. So it would be
the task of an optimizer to rewrite the `Lateral/Bind` to a plain `FILTER(?o =
123)`.
When disabling the SyntaxVarScope check for ElementBind, each ?o gets
injected into the unit table beneath `(extend ((?o 123)) (table unit))` such
as `(extend ((?o 123)) (table (row (?o 456` and the evaluation of OpExtend
discards the incompatible binding - which seems like the natural way it should
be to me.
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1918441638
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +295,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
+commonVars.add(v);
+} else {
+vars.add(v);
+}
+});
+
+List bindings = new ArrayList<>(table.size());
BindingBuilder builder = BindingFactory.builder();
table.iterator(null).forEachRemaining(row->{
-builder.reset();
-builder.addAll(row);
-builder.addAll(binding);
-table2.addBinding(builder.build());
+if (Algebra.compatible(row, binding, commonVars.iterator())) {
Review Comment:
> `SELECT *` is effectively `SELECT ?employee` which works.
Hm, depending on the viewpoint it is the effective query *after*
substitution, but before that, its `SELECT ?department ?employee` as to make
`?department` in-scope such that the substitution could affect the table and
discard its incompatible rows.
I think the scope error is in-line with the SEP7 statement:
> "Disallow syntactic forms that set variables that may already be present
in the current row."
But I think that this is too restrictive, because it prevents several useful
cases for inlining data using VALUES blocks. IMO if substitution leads to
incompatible bindings then they should just be discarded as usual - no? Perhaps
you have corner cases in mind where under this logic the substitution could
become ambiguous and that this might be the reason for the restrictions.
Removing the restrictions on `Element_Bind` and `Element_Data` restrictions
in `checkLATERAL` in order to make the above "department-employee" query work
causes the following test cases to fail:
syntax-lateral-bad-{[04](https://github.com/apache/jena/blob/main/jena-arq/testing/ARQ/Syntax/Syntax-Lateral/syntax-lateral-bad-04),
[05](https://github.com/apache/jena/blob/main/jena-arq/testing/ARQ/Syntax/Syntax-Lateral/syntax-lateral-bad-05),
[06](https://github.com/apache/jena/blob/main/jena-arq/testing/ARQ/Syntax/Syntax-Lateral/syntax-lateral-bad-06),
[08](https://github.com/apache/jena/blob/main/jena-arq/testing/ARQ/Syntax/Syntax-Lateral/syntax-lateral-bad-08)}.arq
For example, `syntax-lateral-bad-04`:
```sparql
SELECT * {
?s ?p ?o
LATERAL { BIND( 123 AS ?o) }
}
```
This is certainly not an efficient way to write a query, but I don't
understand why the query should be syntactically illegal in the first place. To
me it looks like it would only retain bindings where ?o = 123. So it would be
the task of an optimizer to rewrite the `Lateral/Bind` to a plain `FILTER(?o =
123)`.
When disabling the SyntaxVarScope check for ElementBind, each ?o gets
injected into the unit table beneath `(extend ((?o 123)) (table unit))` such
as `(extend ((?o 123)) (table (row (?o 456` and the evaluation of OpExtend
discards the incompatible binding - which seems like the natural way it should
be to me.
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1918441638
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +295,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
+commonVars.add(v);
+} else {
+vars.add(v);
+}
+});
+
+List bindings = new ArrayList<>(table.size());
BindingBuilder builder = BindingFactory.builder();
table.iterator(null).forEachRemaining(row->{
-builder.reset();
-builder.addAll(row);
-builder.addAll(binding);
-table2.addBinding(builder.build());
+if (Algebra.compatible(row, binding, commonVars.iterator())) {
Review Comment:
> `SELECT *` is effectively `SELECT ?employee` which works.
Hm, depending on the viewpoint it is the effective query *after*
substitution, but before that, its `SELECT ?department ?employee` as to make
`?department` in-scope such that the substitution could affect the table and
discard its incompatible rows.
I think the scope error is in-line with the SEP7 statement:
> "Disallow syntactic forms that set variables that may already be present
in the current row."
But I think that this is too restrictive, because it prevents several cases
for inlining data using VALUES blocks. IMO if substitution leads to
incompatible bindings then they should just be discarded as usual - no? Perhaps
you have corner cases in mind where under this logic the substitution could
become ambiguous and that this might be the reason for the restrictions.
Removing the restrictions on `Element_Bind` and `Element_Data` restrictions
in `checkLATERAL` in order to make the above "department-employee" query work
causes the following test cases to fail:
syntax-lateral-bad-{[04](https://github.com/apache/jena/blob/main/jena-arq/testing/ARQ/Syntax/Syntax-Lateral/syntax-lateral-bad-04),
[05](https://github.com/apache/jena/blob/main/jena-arq/testing/ARQ/Syntax/Syntax-Lateral/syntax-lateral-bad-05),
[06](https://github.com/apache/jena/blob/main/jena-arq/testing/ARQ/Syntax/Syntax-Lateral/syntax-lateral-bad-06),
[08](https://github.com/apache/jena/blob/main/jena-arq/testing/ARQ/Syntax/Syntax-Lateral/syntax-lateral-bad-08)}.arq
For example, `syntax-lateral-bad-04`:
```sparql
SELECT * {
?s ?p ?o
LATERAL { BIND( 123 AS ?o) }
}
```
This is certainly not an efficient way to write a query, but I don't
understand why the query should be syntactically illegal in the first place. To
me it looks like it would only retain bindings where ?o = 123. So it would be
the task of an optimizer to rewrite the `Lateral/Bind` to a plain `FILTER(?o =
123)`.
When disabling the SyntaxVarScope check for ElementBind, each ?o gets
injected into the unit table beneath `(extend ((?o 123)) (table unit))` such
as `(extend ((?o 123)) (table (row (?o 456` and the evaluation of OpExtend
discards the incompatible binding - which seems like the natural way it should
be to me.
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1918441638
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +295,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
+commonVars.add(v);
+} else {
+vars.add(v);
+}
+});
+
+List bindings = new ArrayList<>(table.size());
BindingBuilder builder = BindingFactory.builder();
table.iterator(null).forEachRemaining(row->{
-builder.reset();
-builder.addAll(row);
-builder.addAll(binding);
-table2.addBinding(builder.build());
+if (Algebra.compatible(row, binding, commonVars.iterator())) {
Review Comment:
> `SELECT *` is effectively `SELECT ?employee` which works.
Hm, depending on the viewpoint it is the effective query *after*
substitution, but before that, its `SELECT ?department ?employee` as to make
`?department` in-scope such that the substitution could affect the table and
discard its incompatible rows.
I think the scope error is in-line with the SEP7 statement:
> "Disallow syntactic forms that set variables that may already be present
in the current row."
But I think that this is too restrictive, because it prevents inlining data
using VALUES blocks. IMO if substitution leads to incompatible bindings then
they should just be discarded as usual - no? Perhaps you have corner cases in
mind where under this logic the substitution could become ambiguous and that
this might be the reason for the restrictions.
Removing the restrictions on `Element_Bind` and `Element_Data` restrictions
in `checkLATERAL` in order to make the above "department-employee" query work
causes the following test cases to fail:
syntax-lateral-bad-{[04](https://github.com/apache/jena/blob/main/jena-arq/testing/ARQ/Syntax/Syntax-Lateral/syntax-lateral-bad-04),
[05](https://github.com/apache/jena/blob/main/jena-arq/testing/ARQ/Syntax/Syntax-Lateral/syntax-lateral-bad-05),
[06](https://github.com/apache/jena/blob/main/jena-arq/testing/ARQ/Syntax/Syntax-Lateral/syntax-lateral-bad-06),
[08](https://github.com/apache/jena/blob/main/jena-arq/testing/ARQ/Syntax/Syntax-Lateral/syntax-lateral-bad-08)}.arq
For example, `syntax-lateral-bad-04`:
```sparql
SELECT * {
?s ?p ?o
LATERAL { BIND( 123 AS ?o) }
}
```
This is certainly not an efficient way to write a query, but I don't
understand why the query should be syntactically illegal in the first place. To
me it looks like it would only retain bindings where ?o = 123. So it would be
the task of an optimizer to rewrite the `Lateral/Bind` to a plain `FILTER(?o =
123)`.
When disabling the SyntaxVarScope check for ElementBind, each ?o gets
injected into the unit table beneath `(extend ((?o 123)) (table unit))` such
as `(extend ((?o 123)) (table (row (?o 456` and the evaluation of OpExtend
discards the incompatible binding - which seems like the natural way it should
be to me.
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
afs commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1918096258
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +295,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
+commonVars.add(v);
+} else {
+vars.add(v);
+}
+});
+
+List bindings = new ArrayList<>(table.size());
BindingBuilder builder = BindingFactory.builder();
table.iterator(null).forEachRemaining(row->{
-builder.reset();
-builder.addAll(row);
-builder.addAll(binding);
-table2.addBinding(builder.build());
+if (Algebra.compatible(row, binding, commonVars.iterator())) {
Review Comment:
There must be a problem with `SyntaxVarScope.checkLATERAL` because the
`SELECT *` is effectively `SELECT ?employee` which works.
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1916881083
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +295,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
Review Comment:
(See comment on the line containing `Algebra.compatible`)
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1916869473
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +295,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
+commonVars.add(v);
+} else {
+vars.add(v);
+}
+});
+
+List bindings = new ArrayList<>(table.size());
BindingBuilder builder = BindingFactory.builder();
table.iterator(null).forEachRemaining(row->{
-builder.reset();
-builder.addAll(row);
-builder.addAll(binding);
-table2.addBinding(builder.build());
+if (Algebra.compatible(row, binding, commonVars.iterator())) {
Review Comment:
There is potentially another can of worms I was about to open w.r.t. sep7:
Perhaps its worthy of opening an issue and discussion on sparql-dev. The
reason I added the compatibility check was because I had the following query in
mind when writing the tests, which for reasons I do not yet understand is
currently prohibited.
Removing the restrictions in `SyntaxVarScope.checkLATERAL` on `Element_Data`
(and `Element_Bind`) and adding the check for compatible bindings (effectively
filtering bindings of tables down to those compatible with the current row; as
done in the PR) naturally makes the query work.
```sparql
SELECT * {
VALUES ?department {
}
LATERAL {
SELECT * {
VALUES (?department ?employee) {
( )
( )
( )
}
} ORDER BY ?employee LIMIT 1
}
}
```
My expectation:
| department | employee |
||-|
| | |
| | |
Actual result:
Scoping error due to check in `SyntaxVarScope.checkLATERAL` .
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1917059548
##
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:
Renamed to `asRowSet`.
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1916869473
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +295,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
+commonVars.add(v);
+} else {
+vars.add(v);
+}
+});
+
+List bindings = new ArrayList<>(table.size());
BindingBuilder builder = BindingFactory.builder();
table.iterator(null).forEachRemaining(row->{
-builder.reset();
-builder.addAll(row);
-builder.addAll(binding);
-table2.addBinding(builder.build());
+if (Algebra.compatible(row, binding, commonVars.iterator())) {
Review Comment:
There is potentially another can of worms I was about to open w.r.t. sep7:
Perhaps its worthy of opening an issue and discussion on sparql-dev. The
reason I added the compatibility check was because I had the following query in
mind when writing the tests, which for reasons I do not yet understand is
currently prohibited.
Removing the restrictions in `SyntaxVarScope.checkLATERAL` on `Element_Data`
(and `Element_Bind`) and adding the check for compatible bindings (effectively
filtering bindings of tables down to those compatible with the current row; as
done in the PR) naturally makes the query work.
```sparql
SELECT * {
VALUES ?department {
}
LATERAL {
SELECT * {
VALUES (?department ?employee) {
( )
( )
( )
}
} ORDER BY ?employee LIMIT 1
}
}
```
My expectation:
| department | employee |
||-|
| | |
| | |
Actual result:
Scoping error due to check in `SyntaxVarScope.checkLATERAL` .
> If there is a variable in common from substitution it must be the same
term.
For completeness, it could also be UNDEF.
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1916869473
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +295,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
+commonVars.add(v);
+} else {
+vars.add(v);
+}
+});
+
+List bindings = new ArrayList<>(table.size());
BindingBuilder builder = BindingFactory.builder();
table.iterator(null).forEachRemaining(row->{
-builder.reset();
-builder.addAll(row);
-builder.addAll(binding);
-table2.addBinding(builder.build());
+if (Algebra.compatible(row, binding, commonVars.iterator())) {
Review Comment:
There is potentially another can of worms I was about to open w.r.t. sep7:
Perhaps its worthy of opening an issue and discussion on sparql-dev. The
reason I added the compatibility check was because I had the following query in
mind when writing the tests, which for reasons I do not yet understand is
currently prohibited.
Removing the restrictions in `SyntaxVarScope.checkLATERAL` on `Element_Data`
(and `Element_Bind`) and adding the check for compatible bindings (effectively
filtering bindings of tables down to those compatible with the current row; as
done in the PR) naturally makes the query work.
```sparql
SELECT * {
VALUES ?department {
}
LATERAL {
SELECT * {
VALUES (?department ?employee) {
( )
( )
( )
}
} ORDER BY ?employee LIMIT 1
}
}
```
My expectation:
| department | employee |
||-|
| | |
| | |
Actual result:
Scoping error due to check in `SyntaxVarScope.checkLATERAL` .
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1916881083
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +295,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
Review Comment:
(Se comment on Algebra.compatible)
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +295,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
Review Comment:
(Se comment on the line containing `Algebra.compatible`)
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1916869473
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +295,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
+commonVars.add(v);
+} else {
+vars.add(v);
+}
+});
+
+List bindings = new ArrayList<>(table.size());
BindingBuilder builder = BindingFactory.builder();
table.iterator(null).forEachRemaining(row->{
-builder.reset();
-builder.addAll(row);
-builder.addAll(binding);
-table2.addBinding(builder.build());
+if (Algebra.compatible(row, binding, commonVars.iterator())) {
Review Comment:
There is potentially another can of worms I was about to open w.r.t. sep7:
Perhaps its worthy of opening an issue and discussion on sparql-dev. The
reason I added the compatibility check was because I had the following query in
mind, which for reasons I do not yet understand is currently prohibited.
Removing the restrictions in `SyntaxVarScope.checkLATERAL` on `Element_Data`
(and `Element_Bind`) and adding the check for compatible bindings (as done in
the PR) naturally makes the query work.
> SPARQL solution mappings can only have one binding for a variable, and the
current row provides that binding.
Tables could just be filtered to those bindings that are compatible with the
current row - no?
```sparql
SELECT * {
VALUES ?department {
}
LATERAL {
SELECT * {
VALUES (?department ?employee) {
( )
( )
( )
}
} ORDER BY ?employee LIMIT 1
}
}
```
| department | employee |
||-|
| | |
| | |
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1916869473
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +295,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
+commonVars.add(v);
+} else {
+vars.add(v);
+}
+});
+
+List bindings = new ArrayList<>(table.size());
BindingBuilder builder = BindingFactory.builder();
table.iterator(null).forEachRemaining(row->{
-builder.reset();
-builder.addAll(row);
-builder.addAll(binding);
-table2.addBinding(builder.build());
+if (Algebra.compatible(row, binding, commonVars.iterator())) {
Review Comment:
There is potentially another can of worms I was about to open w.r.t. sep7:
Perhaps its worthy of opening an issue and discussion on sparql-dev. The
reason I added the compatibility check was because I had the following query in
mind when writing the tests, which for reasons I do not yet understand is
currently prohibited.
Removing the restrictions in `SyntaxVarScope.checkLATERAL` on `Element_Data`
(and `Element_Bind`) and adding the check for compatible bindings (effectively
filtering bindings of tables down to those compatible with the current row; as
done in the PR) naturally makes the query work.
```sparql
SELECT * {
VALUES ?department {
}
LATERAL {
SELECT * {
VALUES (?department ?employee) {
( )
( )
( )
}
} ORDER BY ?employee LIMIT 1
}
}
```
| department | employee |
||-|
| | |
| | |
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1916869473
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +295,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
+commonVars.add(v);
+} else {
+vars.add(v);
+}
+});
+
+List bindings = new ArrayList<>(table.size());
BindingBuilder builder = BindingFactory.builder();
table.iterator(null).forEachRemaining(row->{
-builder.reset();
-builder.addAll(row);
-builder.addAll(binding);
-table2.addBinding(builder.build());
+if (Algebra.compatible(row, binding, commonVars.iterator())) {
Review Comment:
There is potentially another can of worms I was about to open w.r.t. sep7:
Perhaps its worthy of opening an issue and discussion on sparql-dev. The
reason I added the compatibility check was because I had the following query in
mind, which for reasons I do not yet understand is currently prohibited.
Removing the restrictions in `SyntaxVarScope.checkLATERAL` on `Element_Data`
(and `Element_Bind`) and adding the check for compatible bindings (effectively
filtering bindings of tables down to those compatible with the current row; as
done in the PR) naturally makes the query work.
```sparql
SELECT * {
VALUES ?department {
}
LATERAL {
SELECT * {
VALUES (?department ?employee) {
( )
( )
( )
}
} ORDER BY ?employee LIMIT 1
}
}
```
| department | employee |
||-|
| | |
| | |
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1916869473
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +295,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
+commonVars.add(v);
+} else {
+vars.add(v);
+}
+});
+
+List bindings = new ArrayList<>(table.size());
BindingBuilder builder = BindingFactory.builder();
table.iterator(null).forEachRemaining(row->{
-builder.reset();
-builder.addAll(row);
-builder.addAll(binding);
-table2.addBinding(builder.build());
+if (Algebra.compatible(row, binding, commonVars.iterator())) {
Review Comment:
There is potentially another can of worms I was about to open w.r.t. sep7:
Perhaps its worthy of opening an issue and discussion on sparql-dev. The
reason I added the compatibility check was because I had the following query in
mind, which for reasons I do not yet understand is currently prohibited.
Removing the restrictions in `SyntaxVarScope.checkLATERAL` on `Element_Data`
(and `Element_Bind`) and adding the check for compatible bindings (as done in
the PR) naturally makes the query work.
> SPARQL solution mappings can only have one binding for a variable, and the
current row provides that binding.
```sparql
SELECT * {
VALUES ?department {
}
LATERAL {
SELECT * {
VALUES (?department ?employee) {
( )
( )
( )
}
} ORDER BY ?employee LIMIT 1
}
}
```
| department | employee |
||-|
| | |
| | |
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +295,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
+commonVars.add(v);
+} else {
+vars.add(v);
+}
+});
+
+List bindings = new ArrayList<>(table.size());
BindingBuilder builder = BindingFactory.builder();
table.iterator(null).forEachRemaining(row->{
-builder.reset();
-builder.addAll(row);
-builder.addAll(binding);
-table2.addBinding(builder.build());
+if (Algebra.compatible(row, binding, commonVars.iterator())) {
Review Comment:
There is potentially another can of worms I was about to open w.r.t. sep7:
Perhaps its worthy of opening an issue and discussion on sparql-dev. The
reason I added the compatibility check was because I had the following query in
mind, which for reasons I do not yet understand is currently prohibited.
Removing the restrictions in `SyntaxVarScope.checkLATERAL` on `Element_Data`
(and `Element_Bind`) and adding the check for compatible bindings (as done in
the PR) naturally makes the query work.
```sparql
SELECT * {
VALUES ?department {
}
LATERAL {
SELECT * {
VAL
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1916869473
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +295,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
+commonVars.add(v);
+} else {
+vars.add(v);
+}
+});
+
+List bindings = new ArrayList<>(table.size());
BindingBuilder builder = BindingFactory.builder();
table.iterator(null).forEachRemaining(row->{
-builder.reset();
-builder.addAll(row);
-builder.addAll(binding);
-table2.addBinding(builder.build());
+if (Algebra.compatible(row, binding, commonVars.iterator())) {
Review Comment:
There is potentially another can of worms I was about to open w.r.t. sep7:
Perhaps its worthy of opening an issue and discussion on sparql-dev. The
reason I added the compatibility check was because I had the following query in
mind, which for reasons I do not yet understand is currently prohibited.
Removing the restrictions in `SyntaxVarScope.checkLATERAL` on `Element_Data`
(and `Element_Bind`) and adding the check for compatible bindings (as done in
the PR) naturally makes the query work.
```sparql
SELECT * {
VALUES ?department {
}
LATERAL {
SELECT * {
VALUES (?department ?employee) {
( )
( )
( )
}
} ORDER BY ?employee LIMIT 1
}
}
```
| department | employee |
||-|
| | |
| | |
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1916869473
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +295,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
+commonVars.add(v);
+} else {
+vars.add(v);
+}
+});
+
+List bindings = new ArrayList<>(table.size());
BindingBuilder builder = BindingFactory.builder();
table.iterator(null).forEachRemaining(row->{
-builder.reset();
-builder.addAll(row);
-builder.addAll(binding);
-table2.addBinding(builder.build());
+if (Algebra.compatible(row, binding, commonVars.iterator())) {
Review Comment:
There is potentially another can of worms I was about to open w.r.t. sep7:
Perhaps its worthy of opening an issue and discussion on sparql-dev. The
reason I added the compatibility check was because I had the following query in
mind, which for reasons I do not yet understand is currently prohibited.
Removing the restrictions `SyntaxVarScope.checkLATERAL` on `Element_Bind`
and `Element_Data` and adding the check for compatible bindings (as done in the
PR) naturally makes the query work.
```sparql
SELECT * {
VALUES ?department {
}
LATERAL {
SELECT * {
VALUES (?department ?employee) {
( )
( )
( )
}
} ORDER BY ?employee LIMIT 1
}
}
```
| department | employee |
||-|
| | |
| | |
---
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1916869473
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +295,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
+commonVars.add(v);
+} else {
+vars.add(v);
+}
+});
+
+List bindings = new ArrayList<>(table.size());
BindingBuilder builder = BindingFactory.builder();
table.iterator(null).forEachRemaining(row->{
-builder.reset();
-builder.addAll(row);
-builder.addAll(binding);
-table2.addBinding(builder.build());
+if (Algebra.compatible(row, binding, commonVars.iterator())) {
Review Comment:
There is potentially another can of worms I was about to open w.r.t. sep7:
Perhaps its worthy of opening an issue and discussion on sparql-dev. The
reason I added the compatibility check was because I had the following query in
mind, which for reasons I do not yet understand is currently prohibited.
Removing the restrictions in `SyntaxVarScope.checkLATERAL` on `Element_Data`
(and `Element_Bind`) and adding the check for compatible bindings (as done in
the PR) naturally makes the query work.
```sparql
SELECT * {
VALUES ?department {
}
LATERAL {
SELECT * {
VALUES (?department ?employee) {
( )
( )
( )
}
} ORDER BY ?employee LIMIT 1
}
}
```
| department | employee |
||-|
| | |
| | |
---
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1916869473
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +295,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
+commonVars.add(v);
+} else {
+vars.add(v);
+}
+});
+
+List bindings = new ArrayList<>(table.size());
BindingBuilder builder = BindingFactory.builder();
table.iterator(null).forEachRemaining(row->{
-builder.reset();
-builder.addAll(row);
-builder.addAll(binding);
-table2.addBinding(builder.build());
+if (Algebra.compatible(row, binding, commonVars.iterator())) {
Review Comment:
There is potentially another can of worms I was about to open w.r.t. sep7:
Perhaps its worthy of opening an issue and discussion on sparql-dev. The
reason I added the compatibility check was because I had the following query in
mind, which for reasons I do not yet understand is currently prohibited.
Removing the restrictions `SyntaxVarScope.checkLATERAL` on `Element_Bind`
and `Element_Data` and adding the check for compatible bindings (as done in the
PR) naturally makes the query work.
```sparql
SELECT * { VALUES ?department {
}
LATERAL { SELECT * { VALUES (?department ?employee) {
( )
( )
( )
}
} ORDER BY ?employee LIMIT 1
}
}
```
| department | employee |
||-|
| | |
| | |
---
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1916869473
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +295,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
+commonVars.add(v);
+} else {
+vars.add(v);
+}
+});
+
+List bindings = new ArrayList<>(table.size());
BindingBuilder builder = BindingFactory.builder();
table.iterator(null).forEachRemaining(row->{
-builder.reset();
-builder.addAll(row);
-builder.addAll(binding);
-table2.addBinding(builder.build());
+if (Algebra.compatible(row, binding, commonVars.iterator())) {
Review Comment:
There is potentially another can of worms I was about to open w.r.t. sep7:
Perhaps its worthy of opening an issue and discussion on sparql-dev. The
reason I added the compatibility check was because I had the following query in
mind, which for reasons I do not yet understand is currently prohibited.
Removing the restrictions `SyntaxVarScope.checkLATERAL` on `Element_Bind`
and `Element_Data`) and adding the check for compatible bindings (as done in
the PR) naturally makes the query work.
```sparql
SELECT * { VALUES ?department {
}
LATERAL { SELECT * { VALUES (?department ?employee) {
( )
( )
( )
}
} ORDER BY ?employee LIMIT 1
}
}
```
| department | employee |
||-|
| | |
| | |
---
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1916869473
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +295,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
+commonVars.add(v);
+} else {
+vars.add(v);
+}
+});
+
+List bindings = new ArrayList<>(table.size());
BindingBuilder builder = BindingFactory.builder();
table.iterator(null).forEachRemaining(row->{
-builder.reset();
-builder.addAll(row);
-builder.addAll(binding);
-table2.addBinding(builder.build());
+if (Algebra.compatible(row, binding, commonVars.iterator())) {
Review Comment:
There is potentially another can of worms I was about to open w.r.t. sep7:
Perhaps its worthy of opening an issue and discussion on sparql-dev. The
reason I added the compatibility check was because I had the following query in
mind, which for reasons I do not yet understand is currently prohibited:
```sparql
SELECT * { VALUES ?department {
}
LATERAL { SELECT * { VALUES (?department ?employee) {
( )
( )
( )
}
} ORDER BY ?employee LIMIT 1
}
}
```
| department | employee |
||-|
| | |
| | |
---
So when creating the PR I was surprised to find out that this query is
already considered illegal.
Removing the restrictions `SyntaxVarScope.checkLATERAL` on `Element_Bind`
and `Element_Data`) and adding the check for compatible bindings (as done in
the PR) naturally makes the query work.
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1916869473
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +295,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
+commonVars.add(v);
+} else {
+vars.add(v);
+}
+});
+
+List bindings = new ArrayList<>(table.size());
BindingBuilder builder = BindingFactory.builder();
table.iterator(null).forEachRemaining(row->{
-builder.reset();
-builder.addAll(row);
-builder.addAll(binding);
-table2.addBinding(builder.build());
+if (Algebra.compatible(row, binding, commonVars.iterator())) {
Review Comment:
There is potentially another can of worms I was about to open w.r.t. sep7:
Perhaps its worthy of opening an issue and discussion on sparql-dev. The
reason I added the compatibility check was because I had the following query in
mind, which for reasons I do not understand is currently prohibited:
```sparql
SELECT * { VALUES ?department {
}
LATERAL { SELECT * { VALUES (?department ?employee) {
( )
( )
( )
}
} ORDER BY ?employee LIMIT 1
}
}
```
| department | employee |
||-|
| | |
| | |
---
Removing the restrictions `SyntaxVarScope.checkLATERAL` on `Element_Bind`
and `Element_Data`) and adding the check for compatible bindings (as done in
the PR) naturally makes the query work.
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1916833573
##
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:
Shorthand `emptyDataset()` removed.
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925: URL: https://github.com/apache/jena/pull/2925#discussion_r1916830062 ## 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: I reverted my changes to Query and updated the test case that attempted to modify the query's values block. -- 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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925: URL: https://github.com/apache/jena/pull/2925#discussion_r1916830062 ## 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: I reverted my changes to Query and updated the test case. -- 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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925: URL: https://github.com/apache/jena/pull/2925#discussion_r1916830062 ## 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: Query has been reverted and the test case has been updated. -- 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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
afs commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1916747478
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +295,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
Review Comment:
The table vars can't contain a binding var (or am I missing something?).
That case is caught by the text
https://github.com/w3c/sparql-dev/blob/main/SEP/SEP-0007/sep-0007.md
"Disallow syntactic forms that set variables that may already be present in
the current row."
It's tested in `SyntaxVarScope.checkLATEAL`.
(I removed the `if`, left the `else` and the two tests pass.)
If so, it simplifies to `vars.add(v);`
And hence "vars = tableVars" (copy not needed).
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
afs commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1916747478
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +295,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
Review Comment:
The table vars can't contain a binding var. That case is caught by the text
https://github.com/w3c/sparql-dev/blob/main/SEP/SEP-0007/sep-0007.md
"Disallow syntactic forms that set variables that may already be present in
the current row."
It's tested in `SyntaxVarScope.checkLATEAL`.
(I removed the `if`, left the `else` and the two tests pass.)
If so, it simplifies to `vars.add(v);`
And hence "vars = tableVars" (copy not needed).
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
afs commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1916747478
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +295,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
Review Comment:
The table vars can't contain a binding var. That case is caught by the
https://github.com/w3c/sparql-dev/blob/main/SEP/SEP-0007/sep-0007.md
"Disallow syntactic forms that set variables that may already be present in
the current row."
It's tested in `SyntaxVarScope.checkLATEAL`.
(I removed the `if`, left the `else` and the second test passes).
If so, it simplifies to `vars.add(v);`
And hence "vars = tableVars" (copy not needed).
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +295,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
+commonVars.add(v);
+} else {
+vars.add(v);
+}
+});
+
+List bindings = new ArrayList<>(table.size());
BindingBuilder builder = BindingFactory.builder();
table.iterator(null).forEachRemaining(row->{
-builder.reset();
-builder.addAll(row);
-builder.addAll(binding);
-table2.addBinding(builder.build());
+if (Algebra.compatible(row, binding, commonVars.iterator())) {
Review Comment:
If there is a variable in common from substitution it must be the same term.
So this is a contains relationship. (Given the comments above there may be a
simpler way.)
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1915156203
##
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 variables, List rows) {
-super(variables, rows) ;
+super(Collections.unmodifiableList(variables),
Collections.unmodifiableList(rows)) ;
Review Comment:
The constructor already existed. The wrappers prevent mutation via
`TableData.getRows().add()` which wasn't the case before. Its not perfect, but
from your comment
> It should be "immutable once built"
its a step closer towards preventing incorrect use of `TableData`.
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1915156203
##
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 variables, List rows) {
-super(variables, rows) ;
+super(Collections.unmodifiableList(variables),
Collections.unmodifiableList(rows)) ;
Review Comment:
The constructor already existed. The wrappers prevent mutation via
`TableData.getRows().add()` which wasn't the case before. Its not perfect, but
from your comment
> It should be "immutable once built"
its a step closer towards `TableData` being immutable.
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1915207573
##
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 ) (?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:
As mentioned, I view Table as a Collection and RowSet as an Iterator.
If I am not mistaken, then table's `equals()` method naturally and
succinctly performs an exact comparison (using term equality). For working with
iterators (RowSet/ResultSet) one should of course use ResultSetCompare /
RowSetOps methods.
In this case, one could also write a bit more verbose:
`assertTrue(ResultSetCompare.exactEquals(expected.toRowSet(),
actual.toRowSet());`
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1915207573
##
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 ) (?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:
As mentioned, I view Table as a collection and RowSet as an Iterator.
If I am not mistaken, then table's `equals()` method naturally and
succinctly performs an exact comparison (using term equality). For working with
iterators (RowSet/ResultSet) one should of course use ResultSetCompare /
RowSetOps methods.
In this case, one could also write a bit more verbose:
`assertTrue(ResultSetCompare.exactEquals(expected.toRowSet(),
actual.toRowSet());`
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1915180968
##
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:
I was also thinking that BindingFactory could take care of making bindings
independent. But then again, why should BindingFactory have special logic for
BindingTDB instances? In principle, `BindingTDB.detach` could be implemented as
just setting filling the already present (but unused) cache - so no copy would
be needed then.
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1915180968
##
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:
I was also thinking that BindingFactory could take care of making bindings
independent. But then again, why should BindingFactory have special logic for
BindingTDB instances? In principle, `BindingTDB.detach` could be implemented as
just filling the already present (but unused) cache - so no copy would be
needed then.
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1915112399
##
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 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:
The difference (to my understanding) is that a Table acts as a Collection
and a RowSet as an Iterator.
A Table can thus be seen as a factory for RowSets via `Table.toRowSet()` -
each obtained RowSet is an independent iterator over the Table.
My qualms with RowSet.materialize/rewindable is that these methods operate
on the iterator level; it seemed clean to me to add Table as a collection of
bindings.
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1915156203
##
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 variables, List rows) {
-super(variables, rows) ;
+super(Collections.unmodifiableList(variables),
Collections.unmodifiableList(rows)) ;
Review Comment:
The constructor already existed. The wrappers prevent mutation via
`TableN.getRows().add()` which wasn't the case before. Its not perfect, but
from your comment
> It should be "immutable once built"
its a step closer towards `TableData` being immutable.
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1915112399
##
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 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:
The difference (to my understanding) is that a Table acts as a Collection
and a RowSet as an Iterator.
A Table can thus be seen as a factory for RowSets via `Table.toRowSet()` -
each obtained RowSet is an independent iterator over the Table.
My qualms with RowSet.materialize/rewindable is that these methods operate
on the iterator level; it seemed clean to me to add Table as a collection-level
view over a list of bindings.
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925: URL: https://github.com/apache/jena/pull/2925#discussion_r1915125501 ## 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: Ok, so I'll change back to TableData and update the test case to expect failure when trying to modify the rows of a queries valuesDataBlock directly. -- 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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1915119630
##
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:
It was just a suggestion for a shortcut - I'll remove it.
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
afs commented on PR #2925: URL: https://github.com/apache/jena/pull/2925#issuecomment-2590148039 First part of a review - trying to understand the machinery changes. There seem to be a lot of them and I'm trying to understand what are necessary. -- 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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
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 stream() {
public static RowSet create(QueryIterator qIter, List 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 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,1
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1908641289
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +307,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
+commonVars.add(v);
+} else {
+vars.add(v);
+}
+});
+
+List bindings = new ArrayList<>();
BindingBuilder builder = BindingFactory.builder();
table.iterator(null).forEachRemaining(row->{
-builder.reset();
-builder.addAll(row);
-builder.addAll(binding);
-table2.addBinding(builder.build());
Review Comment:
addBinding checks whether the table's variables need to be updated.
This check is not needed here, so I just collect the bindings in a list
instead and create the table afterwards.
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1908641289
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +307,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
+commonVars.add(v);
+} else {
+vars.add(v);
+}
+});
+
+List bindings = new ArrayList<>();
BindingBuilder builder = BindingFactory.builder();
table.iterator(null).forEachRemaining(row->{
-builder.reset();
-builder.addAll(row);
-builder.addAll(binding);
-table2.addBinding(builder.build());
Review Comment:
addBinding checks whether the table's variables need to be updated.
This check is not needed here, so in the revised code I just collect the
bindings in a list instead and create the table afterwards.
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1908641289
##
jena-arq/src/main/java/org/apache/jena/sparql/engine/iterator/QueryIterLateral.java:
##
@@ -292,17 +307,33 @@ public Op transform(OpTable opTable) {
// By the assignment restriction, the binding only needs to be
added to each row of the table.
Table table = opTable.getTable();
// Table vars.
-List vars = new ArrayList<>(table.getVars());
-binding.vars().forEachRemaining(vars::add);
-TableN table2 = new TableN(vars);
+List tableVars = table.getVars();
+List vars = new ArrayList<>(tableVars);
+
+// Track variables that appear both in the table and the binding.
+List commonVars = new ArrayList<>();
+
+// Index variables in a set if there are more than a few of them.
+Collection tableVarsIndex = tableVars.size() > 4 ? new
HashSet<>(tableVars) : tableVars;
+binding.vars().forEachRemaining(v -> {
+if (tableVarsIndex.contains(v)) {
+commonVars.add(v);
+} else {
+vars.add(v);
+}
+});
+
+List bindings = new ArrayList<>();
BindingBuilder builder = BindingFactory.builder();
table.iterator(null).forEachRemaining(row->{
-builder.reset();
-builder.addAll(row);
-builder.addAll(binding);
-table2.addBinding(builder.build());
Review Comment:
addBinding checks whether the table's variables need to be updated; this
check is not needed here.
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925: URL: https://github.com/apache/jena/pull/2925#discussion_r1908602744 ## 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: Should the valuesDataBlock be mutable or not? One test case written by me some years ago assumed it to be mutable. The test case clones a query and checks that modification of the clone's valuesDataBlock does not affect the original one. But the implementation of TableData suggests that it is intended to be immutable anyway as the addBinding method is overridden to reject updates. As I see it: TableN -> mutable, TableData -> immutable - much like java's List and guava's ImmutableList. -- 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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925: URL: https://github.com/apache/jena/pull/2925#discussion_r1908602744 ## 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: Should the valuesDataBlock be mutable or not? One test case written by me some years ago assumed it to be mutable. The test case clones a query and checks that modification of the clone's valuesDataBlock does not affect the original one. But the implementation of TableData suggests that it is intended to be immutable anyway as the addBinding method is overridden to reject updates. As I see it: TableN -> mutable, TableData -> immutable - much like guava's List and ImmutableList. -- 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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925: URL: https://github.com/apache/jena/pull/2925#discussion_r1908602744 ## 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: Should the valuesDataBlock be mutable or not? One test case written by me some years ago assumed it to be mutable. The test case clones a query and checks that modification of the clone's valuesDataBlock does not affect the original one. But the implementation of TableData suggests to be immutable as the addBinding method is overridden to reject updates. As I see it: TableN -> mutable, TableData -> immutable - much like guava's List and ImmutableList. -- 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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925:
URL: https://github.com/apache/jena/pull/2925#discussion_r1908608373
##
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 variables, List rows) {
-super(variables, rows) ;
+super(Collections.unmodifiableList(variables),
Collections.unmodifiableList(rows)) ;
Review Comment:
TableData appears to be intended as immutable but this was so far not
enforced - one could call getRows() and modify them.
--
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
Re: [PR] GH-2924: Lateral - fixed injection for tables and enhanced QueryExec API for easier testing. [jena]
Aklakan commented on code in PR #2925: URL: https://github.com/apache/jena/pull/2925#discussion_r1908602744 ## 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: Should the valuesDataBlock be mutable or not? One test case written by me some years ago assumed it to be mutable - but the implementation of TableData suggests to be immutable as the addBinding method is overridden to reject updates. As I see it: TableN -> mutable, TableData -> immutable - much like guava's List and ImmutableList. -- 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
