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

CC-1878: Allow for case differences while finding missing columns #400

Merged
merged 3 commits into from
Apr 25, 2018

Conversation

wicknicks
Copy link
Member

Signed-off-by: Arjun Satish arjun@confluent.io

@ghost
Copy link

ghost commented Apr 24, 2018

It looks like @wicknicks hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement here.

Once you've signed reply with [clabot:check] to prove it.

Appreciation of efforts,

clabot

@wicknicks wicknicks force-pushed the CC-1878 branch 3 times, most recently from 65f7f25 to e063363 Compare April 24, 2018 23:05
Signed-off-by: Arjun Satish <arjun@confluent.io>
@ghost
Copy link

ghost commented Apr 24, 2018

It looks like @wicknicks hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement here.

Once you've signed reply with [clabot:check] to prove it.

Appreciation of efforts,

clabot

Copy link
Member

@rhauch rhauch left a comment

Choose a reason for hiding this comment

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

A few suggestions.


if (columnNamesLowerCase.size() != dbColumnNames.size()) {
log.warn("Table has column names which differ only with case sensitivity. Original "
+ "columns={}", dbColumnNames);
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps different wording:

Table has column names that differ from field names only by case. Original ...

@@ -172,6 +172,33 @@ boolean amendIfNecessary(
missingFields.add(field);
}
}
return missingFields;

Copy link
Member

Choose a reason for hiding this comment

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

Maybe return here if missingFields.isEmpty()?

Copy link
Member Author

Choose a reason for hiding this comment

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

This list is used to create ALTER table queries. So we need to return the set here.

Copy link
Member

Choose a reason for hiding this comment

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

Right. If there are no missing fields at this point, why go through the work of checking case-insensitively? Why not return an empty list here?

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, missed that. sorry. fixed this.

Signed-off-by: Arjun Satish <arjun@confluent.io>
Signed-off-by: Arjun Satish <arjun@confluent.io>
Copy link
Member

@rhauch rhauch left a comment

Choose a reason for hiding this comment

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

LGTM.

@wicknicks wicknicks merged commit 41a685c into confluentinc:3.3.x Apr 25, 2018
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