Open
Conversation
This included adding some arguments to old tests and writing a few new ones to test custom join logic.
Now just need to implement the joins when performing searches...
This supports types that need no special processing of the column data returned from the SQLite database and which are single-valued. Further work will be needed to support multi-valued columns. I also need to implement some more sophisticated logic for some of the more complex data types. Basically, any type which overrides the `filter` method will also need an override for the purposes of JOINing.
Moving the left-join needed for multi-columns into a separate method should prove useful for filtering and joining as well.
After my refactoring, prefixes and postfixes are only registered as values once when comparing columns against multiple RHS values. This will work just fine but means that things differ slightly from existing tests.
Member
|
@cmacmackin awesome! It will probably take us a while to review this but I appreciate the effort. |
This allows the order of the additional join to be varied depending on whether filtering or joining primary data. Also renamed getSqlConstantValue to wrapValue, which is more descriptive.
This is known not to work when the RHS of the JOIN is a page title; the SQL ends up broken somehow. It does work when only the LHS is a title though
It now respects order not only for the existing table being joined against, but also for any other tables referenced in the ON clause. This was necessary to get custom joins against page titles to work properly.
These both represent complicated joins which actually require joining against more than 1 table. The page title one is particularly complicated, as it should actually match against two possible values (id or title).
Contributor
Author
|
@splitbrain I've now finished my work on this. I appreciate that this is a very complicated pull request that will take some time to review. I'm not really in a rush. This was just something I started working on months ago and figured I should try to get it finished before tackling the other issues I recently raised. I've written tests for new features that I've added but I can't speak for how well the existing test-suite covers some of my refactoring. |
Contributor
Author
|
Also, I'm not experienced working with databases, so it is possible that some of the queries could be structured better. |
This file contains hidden or 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
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.
This introduces the ability to JOIN schemas in aggregations ON fields other than the page ID. It required some significant refactoring of the SQL generation so will need extensive review.
Fixes #269, fixes #285, fixes #598
For the time being, joins are only supported on single-valued columns. Multi-valued columns represented another layer of complexity on what is already an extremely complicated set of changes. It would probably significantly delay me finishing this work, so I thought it would be best to get what I currently have merged in. Someone can always add support later.
This work involved the following changes:
Search.select()methodselect()and intoselectCol()(which gets called fromselect())getSqlCompareValue()method to the datatype classes. This returns an expression for the contents of the column to be used either in filtering or joining. If need be, it will also insert any other conditional expressions needed into a query's WHERE clause.getAdditionalJoinForComparison()method for the datatype classes. This returns information needed to perform any extra joins required when filtering or joining data. Returning the arguments, rather than setting up the join itself, allows the order of joins to be chosen appropriately for the context.wrapValues()/wrapValue()methods to the datatype classes. These will wrap a value (or array of values) that is being compared against in any additional logic that is needed (e.g., making it lower-case or converting it to digits).filter()only onAbstractBaseTypeand refactor it to usegetSqlCompareValue()andgetAdditionalJoinForComparison()for the type-specific logic.joinCondition()method to construct alternative JOIN ON clauses when generating SQL queries.joinConditionIfAdditionalJoin()method to allow any further type-specific logic for complicated joins (i.e., page titles)QueryBuilder::addLeftJoin(), choose insertion order based on all tables referenced in the ON clause (not just the left table being joined against).