Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Cast pushdown test case refactoring #23737

Merged

Conversation

krvikash
Copy link
Contributor

Description

Follow up of #22728 (comment) and #22951 (comment)

Release notes

(X) This is not user-visible or is docs only, and no release notes are required.

@krvikash krvikash force-pushed the krvikash/cast-pushdown-test-refactor branch 2 times, most recently from b6ba45f to 9dd5bf5 Compare October 10, 2024 10:24
@krvikash
Copy link
Contributor Author

Addressed comments


public CastTestCase
public record FailCastTestCase(String sourceColumn, String castType, String errorMessage, Optional<String> pushdownErrorMessage)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not to use different factory methods for CastTestCase ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think its better to have two separate records for success and failed cases. As targetColumn is not necessary in failed case and errorMessage, pushdownErrorMessage is not required for success case.

Introducing factory method

public record CastTestCase(String sourceColumn, String castType, Optional<String> targetColumn, Optional<String> errorMessage, Optional<String> pushdownErrorMessage)
{
  public CastTestCase
  {
      requireNonNull(sourceColumn, "sourceColumn is null");
      requireNonNull(castType, "castType is null");
      requireNonNull(targetColumn, "targetColumn is null");
      requireNonNull(targetColumn, "errorMessage is null");
      requireNonNull(targetColumn, "pushdownErrorMessage is null");
  }

  public CastTestCase castTestCase(String sourceColumn, String castType, String targetColumn)
  {
      return new CastTestCase(sourceColumn, castType, Optional.of(targetColumn), Optional.empty(), Optional.empty());
  }

  public CastTestCase failingCastTestCase(String sourceColumn, String castType)
  {
      return new CastTestCase(sourceColumn, castType, Optional.empty(), Optional.of("(.*)Cannot cast (.*) to (.*)"), Optional.empty());
  }

  public CastTestCase failingCastTestCase(String sourceColumn, String castType, String errorMessage)
  {
      return new CastTestCase(sourceColumn, castType, Optional.empty(), Optional.of(errorMessage), Optional.empty());
  }

  public CastTestCase failingCastTestCase(String sourceColumn, String castType, String errorMessage, String pushdownErrorMessage)
  {
      return new CastTestCase(sourceColumn, castType, Optional.empty(), Optional.of(errorMessage), Optional.of(pushdownErrorMessage));
  }
}

By having 2 separate records.

public record CastTestCase(String sourceColumn, String castType, String targetColumn)
{
    public CastTestCase
    {
        requireNonNull(sourceColumn, "sourceColumn is null");
        requireNonNull(castType, "castType is null");
        requireNonNull(targetColumn, "targetColumn is null");
    }
}

public record FailCastTestCase(String sourceColumn, String castType, String errorMessage, Optional<String> pushdownErrorMessage)
{
    public FailCastTestCase(String sourceColumn, String castType)
    {
        this(sourceColumn, castType, "(.*)Cannot cast (.*) to (.*)");
    }

    public FailCastTestCase(String sourceColumn, String castType, String errorMessage)
    {
        this(sourceColumn, castType, errorMessage, Optional.empty());
    }

    public FailCastTestCase
    {
        requireNonNull(sourceColumn, "sourceColumn is null");
        requireNonNull(castType, "castType is null");
        requireNonNull(errorMessage, "errorMessage is null");
        requireNonNull(pushdownErrorMessage, "pushdownErrorMessage is null");
    }
}

@krvikash krvikash force-pushed the krvikash/cast-pushdown-test-refactor branch from 9dd5bf5 to d81aea7 Compare October 10, 2024 12:09
@krvikash
Copy link
Contributor Author

Thanks @SemionPar @ssheikin @Praveen2112 for the review. Addressed comments.

@krvikash krvikash force-pushed the krvikash/cast-pushdown-test-refactor branch from d81aea7 to 40a2d89 Compare October 10, 2024 12:11
@krvikash
Copy link
Contributor Author

(Ready for review)

Copy link
Member

@Praveen2112 Praveen2112 left a comment

Choose a reason for hiding this comment

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

LGTM. % change

@krvikash krvikash force-pushed the krvikash/cast-pushdown-test-refactor branch from 40a2d89 to 762da87 Compare October 10, 2024 13:19
@krvikash krvikash force-pushed the krvikash/cast-pushdown-test-refactor branch from 762da87 to 61c371f Compare October 10, 2024 17:55
@krvikash krvikash force-pushed the krvikash/cast-pushdown-test-refactor branch from 61c371f to 346a6cc Compare October 10, 2024 20:43
@krvikash
Copy link
Contributor Author

(rebased with master)

@Praveen2112 Praveen2112 merged commit fdc8a7a into trinodb:master Oct 11, 2024
61 checks passed
@github-actions github-actions bot added this to the 462 milestone Oct 11, 2024
@krvikash krvikash deleted the krvikash/cast-pushdown-test-refactor branch October 11, 2024 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants