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

[BUG] The current definition of AST node Alias is ambiguous and easily confused #974

Closed
LantaoJin opened this issue Dec 9, 2024 · 0 comments · Fixed by #975
Closed

[BUG] The current definition of AST node Alias is ambiguous and easily confused #974

LantaoJin opened this issue Dec 9, 2024 · 0 comments · Fixed by #975
Assignees
Labels
bug Something isn't working enhancement New feature or request

Comments

@LantaoJin
Copy link
Member

LantaoJin commented Dec 9, 2024

What is the bug?
The current class definition of Alias looks

public class Alias extends UnresolvedExpression {

  /** Original field name. */
  private final String name;

  /** Expression aliased. */
  private final UnresolvedExpression delegated;

  /** Optional field alias. */
  private String alias;

When we need to build an Alias object, we actually use its name, for example:

      UnresolvedExpression aggExpression = internalVisitExpression(aggCtx.statsFunction());
      String name =
          aggCtx.alias == null
              ? getTextInQuery(aggCtx)
              : aggCtx.alias.getText();
      Alias alias = new Alias(name, aggExpression);
      aggListBuilder.add(alias);

But, when we need to use this alias name, sometimes we use its alias, for example:

        Expression arg = context.popNamedParseExpressions().get();
        return context.getNamedParseExpressions().push(
                org.apache.spark.sql.catalyst.expressions.Alias$.MODULE$.apply(arg,
                        node.getAlias() != null ? node.getAlias() : node.getName(),
                        NamedExpression.newExprId(),
                        seq(new java.util.ArrayList<String>()),
                        Option.empty(),

Although, there is no bug here since we check its nullable before using it, this still would be easy to introduce bugs in future.

What is the expected behavior?
Refactor this class definition to remove the alias member in Alias.

What is your host/environment?

  • OS: [e.g. iOS]
  • Version [e.g. 22]
  • Plugins

Do you have any screenshots?
If applicable, add screenshots to help explain your problem.

Do you have any additional context?
Add any other context about the problem.

@LantaoJin LantaoJin added bug Something isn't working untriaged and removed untriaged labels Dec 9, 2024
@LantaoJin LantaoJin self-assigned this Dec 10, 2024
@LantaoJin LantaoJin added the enhancement New feature or request label Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant