Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add preliminary test and test framework changes for ExistanceJoin #4697

Merged

Conversation

gerashegalov
Copy link
Collaborator

@gerashegalov gerashegalov commented Feb 5, 2022

  1. Allow matching short TreeNode string against a regex.
  2. which enables us to ensure that the test exhibits ExistenceJoin

Contributes to #589

Signed-off-by: Gera Shegalov gera@apache.org

Allow matching short TreeNode string against a regex. Enables to make
sure that the test exhibits ExistenceJoin

Contributes to NVIDIA#589

Signed-off-by: Gera Shegalov <gera@apache.org>
Signed-off-by: Gera Shegalov <gera@apache.org>
@gerashegalov gerashegalov self-assigned this Feb 5, 2022
@gerashegalov gerashegalov added this to the Jan 31 - Feb 11 milestone Feb 5, 2022
@gerashegalov
Copy link
Collaborator Author

build

@sameerz sameerz added the task Work required that improves the product but is not user facing label Feb 7, 2022
@sameerz sameerz linked an issue Feb 7, 2022 that may be closed by this pull request
2 tasks
@sameerz sameerz removed a link to an issue Feb 7, 2022
2 tasks
Copy link
Collaborator

@revans2 revans2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks fine. I am just a little confused why this needs to be split off from the rest of the ExistanceJoin code. I guess adding in the regular expression check is technically unrelated, but then I would prefer to see that as a separate PR without the xfail test.

If I saw "Add an xfail'd test for ExistenceJoin" in the changelog I would be confused and not know why we did that or what it was all about.

@gerashegalov
Copy link
Collaborator Author

The code looks fine. I am just a little confused why this needs to be split off from the rest of the ExistanceJoin code. I guess adding in the regular expression check is technically unrelated, but then I would prefer to see that as a separate PR without the xfail test.

It does not have to, but I thought I create it as a tiny milestone in the test-first approach. I can keep working on this branch until the feature is done and there is no need for xfail.

If I saw "Add an xfail'd test for ExistenceJoin" in the changelog I would be confused and not know why we did that or what it was all about.

I will make it more clear if we are going to keep the test-only PR

@gerashegalov
Copy link
Collaborator Author

build

@gerashegalov gerashegalov requested a review from jlowe February 7, 2022 22:39
@gerashegalov gerashegalov changed the title Add an xfail'd test for ExistenceJoin Add preliminary test and test framework changes for ExistanceJoin Feb 8, 2022
@gerashegalov gerashegalov merged commit 0d4c496 into NVIDIA:branch-22.04 Feb 8, 2022
@gerashegalov gerashegalov deleted the gerashegalov/issue589 branch February 8, 2022 14:29
@gerashegalov gerashegalov mentioned this pull request Feb 16, 2022
2 tasks
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
task Work required that improves the product but is not user facing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants