Skip to content

Conversation

@tdg5
Copy link
Contributor

@tdg5 tdg5 commented May 15, 2025

The background here is that I'm trying to write some tests for a SQL builder library that targets multiple dialects and I'm trying to use SqlParser in the tests to describe and assert what the expected AST for the generated SQL is. Along the way, I've run into a couple of internal set properties that have required some hacky workarounds and I wondered if you'd be open to removing the internal modifier from some of these setters.

An example of such a workaround is:

public TableFactor.Table MakeTableWithAlias(string tableName, string alias, Dialect dialect)
{
    var fabricatedQuery = $"select * from {tableName} as {alias}";
    var parsedQuery = sqlQueryParser.Parse(sql, dialect: dialect).First().AsSelect() ??
        throw new InvalidOperationException(
            $"Failed to parse SQL as select: {fabricatedQuery}");
    var body = Assert.IsType<SetExpression.SelectExpression>(parsedQuery.Query.Body);
    Assert.NotNull(body.Select.From);
    return body.Select.From.First().Relation as TableFactor.Table ??
        throw new InvalidOperationException(
            $"Failed to parse table with alias: {tableName} as {alias}");
}

This works for the simple case where I just want an alias, but would require other workarounds for other TableFactor-derived records.

There are still 8 other internal set properties, but I don't know enough about them to make any proposals at this time.

src/SqlParser/Ast/ColumnOption.cs:100:        public Keyword? Order { get; internal set; }
src/SqlParser/Ast/ColumnOption.cs:101:        public Keyword? Conflict { get; internal set; }
src/SqlParser/Ast/ColumnOption.cs:102:        public bool Autoincrement { get; internal set; }
src/SqlParser/Ast/CommonTableExpression.cs:14:    public Ident? From { get; internal set; } = From;
src/SqlParser/Ast/HiveFormat.cs:8:    public HiveRowFormat? RowFormat { get; internal set; }
src/SqlParser/Ast/HiveFormat.cs:9:    public Sequence<SqlOption>? SerdeProperties { get; internal set; }
src/SqlParser/Ast/HiveFormat.cs:10:    public HiveIOFormat? Storage { get; internal set; }
src/SqlParser/Ast/HiveFormat.cs:11:    public string? Location { get; internal set; }

Comments are welcome. Thanks in advance!

@TylerBrinks
Copy link
Owner

I'm not clear what the issue is, or what public accessors would change that the internals don't already handle. Generally speaking, the instances are read-only, which also matches the Rust project. Changing individual properties on the AST often invalidates the entire tree making public accessors tricky. There are various syntax features in C# to make clones (e.g. new [object] with {props}) to handle similar cases. Can you elaborate?

@tdg5
Copy link
Contributor Author

tdg5 commented May 16, 2025

@TylerBrinks thanks for the response!

I think I failed you by providing an example of the wrong thing. To be clear, I am 100% in favor of immutable data and don't mean to suggest otherwise (that's why I suggested make Expression.Args have an init setter instead of plain old get... I would have done the same for TableFactor.Alias, but that seemed like a more involved change).

Here's an incomplete example of what I am trying, and failing to do:

List<TableFactor> expectedRelations = [
    new TableFactor.Table("MyAwesomeTable") { Alias = new TableAlias("T1") },
];
Assert.Equal(expectedRelations, parsedQuery.Query.Body.Select.From.Select(_ => _.Relation));

But I can't do that because I am unable to set TableFactor.Table.Alias, so I have to revert to constructing and parsing some dummy SQL to get a TableFactor.Table instance with the necessary alias.

To try to describe another way, I am trying to use the AST data model to declaratively describe the expected structure of the parsed query.

@TylerBrinks TylerBrinks merged commit 4c2841c into TylerBrinks:main May 20, 2025
1 check passed
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