Restrict table alias length to avoid truncation

[fh-mthomson]

Fixes #1096

It was my first time working with snapshot testing (very cool!) but my approach was to review / accept the proposed changes per devtools::test() messaging only but noting that it updated some unrelated sections (e.g., can explain) in case that's undesired.

[mgirlich]

Thanks for your PR!
Do you actually have to use such long table names that you stumbled over this issue? 😱

So far, this only solves parts of the issue. In case of more than two joins there could still be issues with the length of the table names, e.g.

library(dplyr, warn.conflicts = FALSE)
library(dbplyr, warn.conflicts = FALSE)

lf <- lazy_frame(x = 1, .name = "who_uses_such_long_table_names_as_they_are_hard_to_write")
lf2 <- lazy_frame(x = 1, .name = "some_other_table")

left_join(lf, lf, by = "x") %>% 
  left_join(lf2, by = "x")
#> <SQL>
#> SELECT `who_uses_such_long_table_names_as_they_are_hard_to_write...1`.`x` AS `x`
#> FROM `who_uses_such_long_table_names_as_they_are_hard_to_write` AS `who_uses_such_long_table_names_as_they_are_hard_to_write...1`
#> LEFT JOIN `who_uses_such_long_table_names_as_they_are_hard_to_write` AS `who_uses_such_long_table_names_as_they_are_hard_to_write...2`
#>   ON (`who_uses_such_long_table_names_as_they_are_hard_to_write...1`.`x` = `who_uses_such_long_table_names_as_they_are_hard_to_write...2`.`x`)
#> LEFT JOIN `some_other_table`
#>   ON (`who_uses_such_long_table_names_as_they_are_hard_to_write...1`.`x` = `some_other_table`.`x`)

^{Created on 2023-01-13 with reprex v2.0.2}

If you want you can try to fix this (probably in generate_join_table_names()).

[fh-mthomson]

@mgirlich thank you for the review! I've pushed what I think is a more robust approach based on your great feedback. let me know what you think!

Do you actually have to use such long table names that you stumbled over this issue? 😱

Unfortunately, I'm dealing with tables already materialized by another team as {long_project_id_[1, 2, 3, ...}_{very_descriptive_table_name}. Not ideal, but it works! 😅

[mgirlich]

We could consider creating shorter names by abbreviate but I think this should be done in a different PR. I also noticed that abbreviate() loves underscores a bit too much

abbreviate("abbreviate_does_like_underscores_too_much")
#> abbreviate_does_like_underscores_too_much 
#>                                    "a___"

^{Created on 2023-02-10 with reprex v2.0.2}

[hadley]

I don't understand what this comment is saying. Can you clarify?

[fh-mthomson]

There is seemingly a fundamental limitation to abbreviate() which does not allow for both (a) max length to be binding and (b) names to be guaranteed unique.

From the documentation:

strict | logical: should minlength be observed strictly? Note that setting strict = TRUE may return non-unique strings.

I've opted for strict = FALSE to preserve uniqueness (since dbplyr will assert as much downstream, anyway), but there remains a non-zero risk that aliases still exceed the database limit. As one mitigation, I've updated the method to both.sides which tends to add a bit more scrambling (slightly less obvious alias names) but the resultant length is more likely to be aligned with the requested length.

I've spent a decent amount of time playing with combinations of args (including discussing with ChatGPT) but couldn't come up with a fully-error proof solution using abbreviate or an alternate function.

[hadley]

That table name is 68 characters long?

[fh-mthomson]

After using method = "both.sides" in abbreviate(), this case is no longer an issue. This is a good example where method = "left.keep" preserves too much of the LHS at the expense of having unnecessary length.

[hadley]

Given that you want to make specific assertions about the length of the generate table names, is it possible to avoid the snapshot tests in favour of something more specific?

[fh-mthomson]

Good thinking! Added some to look at the actual values from generate_join_table_names(); this catches the error observed above.

[hadley]

Thanks for working on this!