re-enable simple-subject-crawl for some queries #340
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…wl strategy not yet supported
allows us to write tests with plain vectors/maps instead of creating records
mpoffald
commented
Jan 17, 2023
| @@ -16,18 +18,17 @@ | |||
| (defn- subjects-chan | |||
| "Returns chan of subjects in chunks per index-leaf | |||
| that can be pulled as needed based on the selection criteria of a where clause." | |||
| [{:keys [conn novelty t] :as db} error-ch vars {:keys [p o idx p-ref?] :as _where-clause}] | |||
| [{:keys [conn novelty t] :as db} error-ch vars {:keys [p o p-ref?] :as _where-clause}] | |||
There was a problem hiding this comment.
This p-ref? is never present now, but I left it in case it should be re-enabled?
Fwiw it looks like it was removed in fc90f94 (unless it got moved elsewhere in that file and I didn't catch it). This was well before the recent parsing rewrite
There was a problem hiding this comment.
p-ref? is a legacy concept. Before a property/predicate could be a ref always, or never. Now we support multiple datatype values for any predicate, we cannot know upfront if a predicate has refs or scalar values, or a mix of both.
bplatz
reviewed
Jan 17, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #309
Updates our query-reparsing code for the simple-subject-crawl execution strategy to handle the new format output by parsing, and reintroduces the use of this strategy in some cases (see caveats below).
Also adds some tests that exercise the new parsing pipeline.
I tried to make it as minimally-invasive as possible, so I opted to reparse into something the downstream code could handle, rather than trying to thread updated logic throughout the execution.
notes and caveats
Queries with filters,
iritype clauses, andvarsbindings are excluded from simple-subject-crawlThese used to be supported, but in the interest of time, they were not included in this PR.
For now these will just be rejected by simple-subject-crawl code and executed like regular queries.
Only tests in clj, due to known issues with cljs testing
There's a lot of cleanup that could be done throughout the ssc execution code (read: dead code, map keys that no longer exist), but I punted on that for now
Queries with recursive predicates and fullText predicates don't parse at all right now (Queries with recursive predicates do not parse #339). These were previously ineligible for simple-subject-crawl, so I've attempted to reject these in re-parsing based on how they should parse now, but it's not tested.