Skip to content

[CALCITE-6707] Type inference for CHR function is wrong#4067

Merged
mihaibudiu merged 1 commit intoapache:mainfrom
mihaibudiu:issue6707
Nov 28, 2024
Merged

[CALCITE-6707] Type inference for CHR function is wrong#4067
mihaibudiu merged 1 commit intoapache:mainfrom
mihaibudiu:issue6707

Conversation

@mihaibudiu
Copy link
Contributor

No description provided.

public static final SqlFunction CHR =
SqlBasicFunction.create("CHR",
ReturnTypes.CHAR,
ReturnTypes.CHAR.andThen(SqlTypeTransforms.TO_NULLABLE),
Copy link
Contributor

@dssysolyatin dssysolyatin Nov 27, 2024

Choose a reason for hiding this comment

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

It would be good to extract ReturnTypes.CHAR.andThen(SqlTypeTransforms.TO_NULLABLE) to ReturnTypes how it was done for other return-type inference strategies.

Example for boolean:

/**
   * Type-inference strategy whereby the result type of a call is Boolean,
   * with nulls allowed if any of the operands allow nulls.
   */
  public static final SqlReturnTypeInference BOOLEAN_NULLABLE =
      BOOLEAN.andThen(SqlTypeTransforms.TO_NULLABLE);

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 actually think that the naming of these strategies is wrong, "NULLABLE" suggests that the return type is always NULLABLE. It should be called perhaps NULLABLE_IF_NECESSARY.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you are right, but probably NULLABLE_IF_ARGS or NULLABLE_IF_OPERANDS is better.

There are only two hard things in Computer Science: cache invalidation and naming things.
-- Phil Karlton

Lately, I've been thinking that naming is a harder than invalidation 😅

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 have created a new SqlReturnTypeInference, as you have suggested, but I didn't follow the existing naming pattern. I think this name is better.

Signed-off-by: Mihai Budiu <mbudiu@feldera.com>
@sonarqubecloud
Copy link

@mihaibudiu mihaibudiu merged commit 22e8b03 into apache:main Nov 28, 2024
@mihaibudiu mihaibudiu deleted the issue6707 branch November 28, 2024 06:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants