Issue #2788 - EQL: distinct notions of equality for AST nodes - #2811
Open
homedirectory wants to merge 28 commits into
Open
Issue #2788 - EQL: distinct notions of equality for AST nodes#2811homedirectory wants to merge 28 commits into
homedirectory wants to merge 28 commits into
Conversation
This is required for alpha-equivalance tests. It enables the same query to be transformed with source IDs generated from different sequences.
Attaching a reason to noMatch is not required at present.
Rename every typed `visit(T, T, S)` facade after its node type (e.g., `maxOf` for `MaxOf3`), leaving `visit(Object, Object, S)` as the sole dispatch method. As a result, every recursive sub-visit now routes through the dispatch method, so traversal is always resolved by dynamic dispatch on the child's runtime type. This makes `visit` a universal interceptor. Subclass overrides and their `super.<facade>` calls are renamed to match; `this.yield(...)` is qualified because `yield` is a restricted identifier.
…ad of Object.equals
The main reason for this is to strengthen the invariant -- uniqueness of all stage 3 nodes. With the previous structure, OrderBy3.yield was violating the invariant as it was being shared with the enclosing query's Yields3 (see OrderBy2.transform). Note that IOrderBy3.Yield does not reference a Yield node anymore, but only its name. This shows how a node can reference another without holding a copy of it. The choice of a sealed hierarchy over modifying the existing OrderBy3 record is not just a style preference. It is necessary to align with the dichotomy "nodes vs data". This is exemplified by changes to AbstractSameShapeVisitor: the two order-by variants have separate methods. Now consider how the visitor would look like if we preserved OrderBy3 instead. ``` record OrderBy3 (@nullable ISingleOperand3 operand, @nullable String yieldName, @nullable String yieldColumn, boolean isDesc) class AbstractSameShapeVisitor { public R orderBy(OrderBy3 x, OrderBy3 y, S state) { if (x.operand() != null && y.operand() != null) { return visit(x.operand(), y.operand(), state); } // BEFORE else if (x.yield() != null && y.yield() != null) { return visit(x.yield(), y.yield(), state); } // AFTER else if (x.yieldName() != null && y.yieldName() != null) { return defaultValue(x, y, state); // OR return visit(x.yieldName(), y.yieldName(), state); } // END else { return noMatch(x, y, state); } } } ``` In the AFTER section, there are 2 implementation choices, both inadequate: 1. defaultValue. This method should be called only for leaf nodes, which OrderBy3 is not. This results in visitors that override all methods receiving an exception from defaultValue(). They are thus forced to override orderBy() completely. 2. visit(x.yieldName(), ...). This is a type error: visit applies to INode3, but yieldName is a String. Widening the parameter type to Object just to accommodate this one String is not warranted.
This again ensure that all nodes within an AST are unique. In the previous version, the same ISource3 could be reached twice -- from IJoinNode3 and Prop3. Although the existing visitors took care to access only Prop3.source.id(), this had always been merely a matter of discipline.
homedirectory
marked this pull request as ready for review
August 4, 2026 13:18
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
1 participant
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.
Resolve #2788
To be completed by the pull request creator
This section should be completed with reference to section Preparing PR of the Code and PR reviews wiki page.
Create the pull request as a draft by tapping the dropdown arrow on the 'Create pull request' button under the pull request description (below the text box where this description is being edited) and changing the default
Create pull requesttoDraft pull request.Or, if the pull request has already been created, convert it to draft by tapping the "Convert to draft" link beneath the "Reviewers" section.
A self-review of all changes has been completed, and the changes are in sync with the issue requirements.
Changes to the requirements have been reflected in the issue description.
Any "leftovers" such as sysouts, printing of stack traces, and any other "temporary" code, have been removed.
Minor refactorings, such as renamings, extraction of constants, etc., have been addressed.
Developer documentation (e.g., comments, Javadoc), have been provided where required.
New Java tests have been written or existing tests adjusted, if required, to cover the new functionality.
All existing and new Java tests pass successfully by running them with Maven.
All existing and new Web tests pass successfully.
Changes have been inspected for possible NPE situations, and the changes are sufficiently defensive.
The correct base branch has been selected for these changes to be merged into.
The latest changes from the base branch have already been merged into this feature branch (and tested).
Added a change overview to the issue description or as a wiki page, referenced in the issue description.
Some issues might be very descriptive and serve in place of a wiki page.
In such cases consider adding label
Wiki liketo the issue.This pull request does not contain significant changes, and at least one appropriate reviewer has been selected.
The
In progresslabel has been removed from the issue.The
Pull requestlabel has been added to the issue.The pull request has been made ready for review by tapping the "Ready for review" button below the list of commits on the pull request page.
To be completed by the pull request reviewer
This section should be completed with reference to section Performing PR review of the Code and PR reviews wiki page.
The
In progresslabel has been added to the pull request in GitHub.The issue requirements have been read and understood (along with any relevant emails and/or Slack messages).
The correct base branch is specified, and that base branch is up-to-date in the local source.
The issue branch has been checked out locally, and had the base branch merged into it.
All automated tests pass successfully.
Ensure the implementation satisfies the functional requirements.
Ensure that code changes are secure and align with the established coding practices, including code formatting and naming conventions.
Ensure that code changes are documented and covered with automated tests as applicable.
Ensure that code changes are well-suited for informal reasoning.
Ensure that changes are documented for the end-user (a software engineer in the case of TG, or an application user in the case of TG-based applications).
If there are significant changes (described above), special attention has been paid to them.
Marked the task items in section "Significant changes" as completed to indicate that corresponding changes have been reviewed, improved if necessary, and approved.
The issue or issues addressed by the pull request are associated with the relevant release milestone.
To be completed by the pull request reviewer once the changes have been reviewed and accepted
The changes have been merged into the base branch (unless there is a specific request not to do so, e.g., they are to be released to SIT).
The issue branch has been deleted (unless the changes have not been merged - see above, or there is a specific request not to do so).
The
In progresslabel has been removed from the pull request.The
Pull requestlabel has been removed from the issue.