-
Notifications
You must be signed in to change notification settings - Fork 14k
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
[fix] handle null column_name in druid/sqla models #7063
[fix] handle null column_name in druid/sqla models #7063
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7063 +/- ##
==========================================
+ Coverage 64.57% 64.57% +<.01%
==========================================
Files 422 422
Lines 20576 20577 +1
Branches 2251 2251
==========================================
+ Hits 13286 13287 +1
Misses 7168 7168
Partials 122 122
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@graceguo-supercat the issue here is we would be replacing None
with an empty string in the sorted list. We want to keep the sorted data intact so the following should work:
sorted([c.colum_name for c in self.columns], key=lambda x: x or '')
2e7692c
to
9e9b16d
Compare
for s in sorted(self.column_names): | ||
# self.column_names return sorted column_names | ||
for s in self.column_names: | ||
s = str(s or '') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think your investigation surfaced that the are columns without a name which seems bad. I can look into why there are columns without names and produce a migration if needed.
I'm a little perplexed what order_by_choices
means for empty strings and how this is used.
@john-bodley I found an example, it is used by
|
@graceguo-supercat thanks for trouble shooting this. I plan on creating a migration tomorrow to remedy this issue which is related to ill-defined data as opposed to an underlying issue with the code. |
@graceguo-supercat I think #7067 should resolve these issues. |
maybe useful for a quick solution in 0.31 |
9e9b16d
to
9857d04
Compare
when sqla table column is null, see following exception:
may related with #5445
@john-bodley @michellethomas @soboko @mistercrunch