Skip to content

[CALCITE-3514] LatticeTest fails when JdbcSchema re-computes JdbcTables#1594

Open
DonnyZone wants to merge 1 commit into
apache:mainfrom
DonnyZone:JdbcTable
Open

[CALCITE-3514] LatticeTest fails when JdbcSchema re-computes JdbcTables#1594
DonnyZone wants to merge 1 commit into
apache:mainfrom
DonnyZone:JdbcTable

Conversation

@DonnyZone

Copy link
Copy Markdown
Contributor

As illustrated in CALCITE-3514, we need to implement JdbcTable's equals() and hashCode() methods.

sourceExpressionList, flattened);
}

@Override public boolean equals(Object obj) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

even though this is trying to fix CALCITE-3514, because this is a general purpose equals implementation, it should checks equality of all fields of this class. Same for hashcode.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I disagree. Two tables are identical if they have the same name, schema etc. No need to check columns.

The "equals" contract does not say you have to check every field.

@amaliujia amaliujia Nov 18, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

O I see. So if schema name is the same then it guarantees that two tables will be the same (e.g. no need to check the schema), in that case it would make sense.

I mentioned every field in checking just because I made no assumption beyond this function. Indeed "equals" not require what is checked.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I add a variable in JdbcSchema to trigger the issue.

@danny0405
danny0405 force-pushed the master branch 2 times, most recently from 80f411d to ca27fe9 Compare November 30, 2019 07:52
@vlsi
vlsi force-pushed the master branch 2 times, most recently from 49cb002 to 8768a23 Compare December 29, 2019 12:07
@julianhyde
julianhyde force-pushed the main branch 2 times, most recently from 8a5cf83 to cf7f71b Compare June 8, 2023 21:21
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