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

support multi-inherit #2738

Merged
merged 8 commits into from
Nov 12, 2024
Merged

support multi-inherit #2738

merged 8 commits into from
Nov 12, 2024

Conversation

jycor
Copy link
Contributor

@jycor jycor commented Nov 8, 2024

Copy link
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

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

See comments on Doltgres PR, I would just fix this to work correctly now if possible

Copy link
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

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

LGTM but Max should take a look at that outscope, it's suspicious.

b.qFlags.Set(sql.QFlagSetDatabase)

outScope.setTableAlias(newTableName)
Copy link
Member

Choose a reason for hiding this comment

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

I think this is probably incorrect strictly speaking, as you'll only have the columns in the last table in the list in this scope. Not sure when it matters though. @max-hoffman ?

Copy link
Contributor

Choose a reason for hiding this comment

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

it's definitely an abuse of the model...a more reasonable pass might accumulate the table cols into a top-level output scope or return an empty scope

Copy link
Contributor

@max-hoffman max-hoffman left a comment

Choose a reason for hiding this comment

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

It's hard to understand what the desired semantics are with the current organization. Some kind of doc string/PR description plus code compartmentalization would probably be worthwhile and make it easier for me to help.

Copy link
Contributor

@max-hoffman max-hoffman left a comment

Choose a reason for hiding this comment

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

I still find several things suspicious and untested, it's possible they are correct for postgres and tested elsewhere. If I had to fix a bug in this code the smaller functions help, but semantically speaking it's still a little vague what should be supported

}

var hasSkippedCols bool
for _, col := range pkSch.Schema {
Copy link
Contributor

Choose a reason for hiding this comment

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

is explicit schema not incompatible with the LIKE list?

Copy link
Contributor

Choose a reason for hiding this comment

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

do we have tests for this?

b.handleErr(err)
// if a column was skipped due to duplicates, don't copy over PK ords, idxDefs, or checkDefs
// since they might be incorrect
if hasSkippedCols {
Copy link
Contributor

Choose a reason for hiding this comment

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

are the semantics really this simple? seems like you'd need way more testing to figure out correct behavior

b.qFlags.Set(sql.QFlagSetDatabase)

outScope.setTableAlias(newTableName)
Copy link
Contributor

Choose a reason for hiding this comment

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

it's definitely an abuse of the model...a more reasonable pass might accumulate the table cols into a top-level output scope or return an empty scope

@jycor jycor merged commit 81b13e8 into main Nov 12, 2024
8 checks passed
@jycor jycor deleted the james/multi-inherit branch November 12, 2024 00:22
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