Skip to content

Conversation

@Expurple
Copy link
Member

@Expurple Expurple commented Jul 29, 2025

Extracted the foo.bar.baz qualification logic that was duplicated across several variants of ColumnRef and TableRef.

I need this to implement schema.type: #827, #922

I'll also need this to implement schema.function later: #795 (reply in thread)

There are also immediate benefits. IMO, ColumnRef and TableRef (and their pattern-matching) look considerably cleaner now. The database. prefix is guaranteed to be supported everywhere where it may be needed.

The detailed changelog is in the diff. I fully preserved (and documented) the existing codegen behavior, as evidenced by unchanged output in tests. If this documented behavior looks suspicious (it kinda does to me), let's fix that later in a separate PR. Here, the only changes in tests are the refactoring that's needed for the new variants to compile.

This is a relatively big breaking change for those users who dig within the internals of ColumnRef, TableRef, and SchemaTable. But that's very low-level and should be rare. My work project never does that, despite depending on SeaORM/SeaQuery/SQLx very closely and frequently getting generic or low-level.

PR Info

@Expurple Expurple requested a review from tyt2y3 July 29, 2025 14:17
/// Table identifier with database and schema prefix and alias
DatabaseSchemaTableAlias(DynIden, DynIden, DynIden, DynIden),
/// A table identifier. Potentially qualified. Potentially remaned using an alias
Table(TableName, Option<DynIden>),
Copy link
Member Author

@Expurple Expurple Jul 29, 2025

Choose a reason for hiding this comment

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

Ideally, I'd use named fields for clarity (alias: Option<DynIden> and so on).

But I chose to keep everything consistent with the other existing variants and not touch those. SeaQuery 1.0 breaks a lot of things already

@Huliiiiii
Copy link
Member

Maybe we should also add corresponding borrowed types?
Here's what I saw in the codebase:

if !from.is_empty()
    && let Some(table) = table
    && let TableRef::Table(table) = table.deref()
{
    self.prepare_column_ref(&ColumnRef::TableColumn(table.clone(), column.clone()), sql);
} else {
    self.prepare_iden(column, sql);
}

Due to the lack of such a type, many places require unnecessary clones.

@Expurple
Copy link
Member Author

Expurple commented Aug 1, 2025

I wouldn't worry too much about these clones. They were SeaRc clones with the old DynIden system, and they are Cow clones now (Borrowed static references most of the time). Both are "shallow" and cheap.

Even in the rare worst case with dynamic Owned strings and a fully-qualified ColumnName, it's still just 4 short strings. A fairly shallow clone. (Databases probably don't allow long identifiers, and no one probably uses those anyway)

@Huliiiiii
Copy link
Member

Oh, I forgot about that.

@Expurple Expurple merged commit aefb97b into SeaQL:master Aug 2, 2025
20 checks passed
Expurple added a commit that referenced this pull request Aug 13, 2025
There was some annoying repetition in this area. `TableName` and
`ColumnName` each had three copy-pasted `From` impls. Now they have one,
which will be easier to copy-paste when I add `TypeName` and
`FunctionName`. I came up with this while re-doing
#922 to add `TypeName`.

Related to #927.

`types.rs` is getting too long and starting to have visible disjoint
"submodules", so I started splitting it.

## PR Info

- Dependents:
  - #922

## New Features

- [x] The new traits are technically an addition, but I wouldn't
advertise it. They are not meant to be used directly. I've documented
this on the traits.
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.

3 participants