Remove SyntheticColumnHandleBuilder #19134
Conversation
kokosing
force-pushed
the
origin/master/002_synthentic_hell
branch
from
ce22aec to
efa4d55
Compare
This entity was not adding much value, it had only single and simple implementation. However it introduced complexity: - changed many constructors APIs - protected field DefaultJdbcMetadataFactory - allowed to introduce an override of SyntheticColumnHandleBuilder which on it is own is is way more complex that single package private method
kokosing
force-pushed
the
origin/master/002_synthentic_hell
branch
from
efa4d55 to
5a6c9c6
Compare
| assertThat(createSyntheticColumn(column("column_with_over_twenty_characters"), 100).getColumnName()) | ||
| .isEqualTo("column_with_over_twenty_ch_100"); | ||
| assertThat(createSyntheticColumn(column("column_with_over_twenty_characters"), Integer.MAX_VALUE).getColumnName()) | ||
| .isEqualTo("column_with_over_tw_2147483647"); |
There was a problem hiding this comment.
minor: are we using soft assertions to understand which test cases actually fail? (as we replaced separate test with a single one)
There was a problem hiding this comment.
What is soft assertion?
There was a problem hiding this comment.
https://joel-costigliola.github.io/assertj/core/api/org/assertj/core/api/SoftAssertions.html
They don't fail on the first failure in the test, but try to go through all assertions in the test and only then show a report of what failed.
There's a TestNG equivalent, but I'm not an expert on TestNG ;)
There was a problem hiding this comment.
Thank you for reference. I think regular assertions are fine. We don't use soft assertions anywhere so far, and I think there is no reason to surprise people here by doing something different than elsewhere.
I also removed using DataProvider. Copying code in tests could be sometimes better over DRY, because code is more readable. Here to be super tester, we could extract each assertion to separate test method, because they are independent. However that would be overkill too due boilerplate code (DataProvider potentially would better in that case?). Putting assertion in single method is kind of middle group. We do that in Trino where we put related assertion to a single method.
TL;DR; it is all about the taste and habits
Remove SyntheticColumnHandleBuilder
This entity was not adding much value, it had only single and simple
implementation. However it introduced complexity:
which on it is own is is way more complex that single package
private method