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.

Sign up for GitHub

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

Jump to bottom

Use original display name for fields in uploaded tables #47303

Merged
merged 7 commits into from
Sep 2, 2024
Merged

Use original display name for fields in uploaded tables #47303

merged 7 commits into from
Sep 2, 2024

Conversation

crisptrutski
Copy link
Contributor

Description

It's not a great experience when using foreign languages to have obfuscated field names for CSV uploads.

This change makes sure we preserve the pre-munged name in the display name.

crisptrutski
crisptrutski commented Aug 27, 2024
View reviewed changes
src/metabase/upload.clj
@@ -68,6 +70,29 @@
(let [column-limit (some-> driver driver/column-name-length-limit)]
(min-safe column-limit (max-bytes :model/Field :name))))

(defn truncate-utf8-bytes
Copy link
Contributor Author

@crisptrutski crisptrutski Aug 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is complex and a bit slow, but we should almost never need to execute the non-trivial branch. Perhaps there's something built in that I just couldn't find.

@crisptrutski crisptrutski requested a review from a team August 27, 2024 12:46
@crisptrutski crisptrutski self-assigned this Aug 27, 2024
@crisptrutski crisptrutski added .Team/BackendComponents also known as BEC backport Automatically create PR on current release branch on merge labels Aug 27, 2024
piranha
piranha approved these changes Aug 27, 2024
View reviewed changes
Copy link
Contributor

@piranha piranha left a comment

Choose a reason for hiding this comment

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

I'm not an expert in that part of the code base, but changes seem rational.

tsmacdonald
tsmacdonald reviewed Aug 27, 2024
View reviewed changes
test/metabase/upload_test.clj Outdated
[["Obi-Wan Kenobi" nil]]
[["Everything" "omega"]]))
(set (rows-for-table table)))))
(io/delete-file file)))))))))
Copy link
Member

Choose a reason for hiding this comment

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

Should probably be in a finally so that it runs if the test fails, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yip. Blaming the copy pasta, and will be better to bulk fix across the suite.

tsmacdonald
tsmacdonald approved these changes Aug 27, 2024
View reviewed changes
Copy link
Member

@tsmacdonald tsmacdonald left a comment

Choose a reason for hiding this comment

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

That imperative let in update-with-csv! gets thornier and thornier, but that's not really your fault. Looks good to me

@crisptrutski
Copy link
Contributor Author

That imperative let in update-with-csv! gets thornier and thornier, but that's not really your fault. Looks good to me

This test suite can use a big clean-up, and I think there's a really practical reason to do so!

I think it's one of the biggest contributors to CI runtime as it really hammers the databases, especially redshift. Most of these tests don't need to touch the database.

@crisptrutski crisptrutski force-pushed the csv-display-names branch 2 times, most recently from 04d90e5 to 628be16 Compare August 28, 2024 19:08
Base automatically changed from csv-non-ascii to master August 28, 2024 21:59
@crisptrutski crisptrutski force-pushed the csv-display-names branch 2 times, most recently from 7591d8c to 45c4a23 Compare August 29, 2024 08:01
@crisptrutski crisptrutski enabled auto-merge (squash) September 2, 2024 08:28
@crisptrutski crisptrutski merged commit cbaaa08 into master Sep 2, 2024
119 checks passed
@crisptrutski crisptrutski deleted the csv-display-names branch September 2, 2024 10:53
github-automation-metabase pushed a commit that referenced this pull request Sep 2, 2024
@crisptrutski
Use original display name for fields in uploaded tables (#47303)
fa0ca9c
@github-automation-metabase github-automation-metabase mentioned this pull request Sep 2, 2024
Merged
github-automation-metabase added a commit that referenced this pull request Sep 2, 2024
@github-automation-metabase @crisptrutski
🤖 backported "Use original display name for fields in uploaded tables" (
a9364fd 
#47507)

* Use original display name for fields in uploaded tables (#47303)

* Rename

---------

Co-authored-by: Chris Truter <crisptrutski@users.noreply.github.com>
Co-authored-by: Chris Truter <chris@metabase.com>
@github-actions github-actions bot added this to the 0.50.24 milestone Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@piranha piranha piranha approved these changes

@tsmacdonald tsmacdonald tsmacdonald approved these changes

@camsaul camsaul Awaiting requested review from camsaul camsaul is a code owner

Assignees

@crisptrutski crisptrutski

Labels
backport Automatically create PR on current release branch on merge .Team/BackendComponents also known as BEC
Projects
None yet
Milestone
0.50.24
Development

Successfully merging this pull request may close these issues.
3 participants
@crisptrutski @piranha @tsmacdonald