have file_stem accept a full path #284

mattseddon · 2024-08-13T03:15:30Z

Follow up to https://github.com/iterative/datachain/pull/101/files#r1697409039 and https://github.com/iterative/datachain/pull/101/files#r1709153373

This PR updates the compiled file_stem function to accept a full path instead of expecting users to write path.file_stem(path.name(C("file.path"))). This change leads to (slightly) better SQL being generated as well.

For comparison I've put the current and newly generated SQL below:

Before PR

              rtrim(
                substr(
                  ltrim(
                    substr(
                      source__file__path,
                      length(
                        rtrim(
                          rtrim(
                            source__file__path,
                            replace(source__file__path, :param_1, :param_2)
                          ),
                          :param_1
                        )
                      ) + :length_16
                    ),
                    :param_1
                  ),
                  :substr_2,
                  length(
                    ltrim(
                      substr(
                        source__file__path,
                        length(
                          rtrim(
                            rtrim(
                              source__file__path,
                              replace(source__file__path, :param_1, :param_2)
                            ),
                            :param_1
                          )
                        ) + :length_17
                      ),
                      :param_1
                    )
                  ) - CASE WHEN (
                    instr(
                      ltrim(
                        substr(
                          ltrim(
                            substr(
                              source__file__path,
                              length(
                                rtrim(
                                  rtrim(
                                    source__file__path,
                                    replace(source__file__path, :param_1, :param_2)
                                  ),
                                  :param_1
                                )
                              ) + :length_18
                            ),
                            :param_1
                          ),
                          length(
                            rtrim(
                              rtrim(
                                ltrim(
                                  substr(
                                    source__file__path,
                                    length(
                                      rtrim(
                                        rtrim(
                                          source__file__path,
                                          replace(source__file__path, :param_1, :param_2)
                                        ),
                                        :param_1
                                      )
                                    ) + :length_19
                                  ),
                                  :param_1
                                ),
                                replace(
                                  ltrim(
                                    substr(
                                      source__file__path,
                                      length(
                                        rtrim(
                                          rtrim(
                                            source__file__path,
                                            replace(source__file__path, :param_1, :param_2)
                                          ),
                                          :param_1
                                        )
                                      ) + :length_20
                                    ),
                                    :param_1
                                  ),
                                  :param_1,
                                  :param_2
                                )
                              ),
                              :param_1
                            )
                          ) + :length_21
                        ),
                        :param_1
                      ),
                      :param_3
                    ) = :instr_2
                  ) THEN :param_5 ELSE length(
                    ltrim(
                      substr(
                        ltrim(
                          substr(
                            source__file__path,
                            length(
                              rtrim(
                                rtrim(
                                  source__file__path,
                                  replace(source__file__path, :param_1, :param_2)
                                ),
                                :param_1
                              )
                            ) + :length_22
                          ),
                          :param_1
                        ),
                        length(
                          rtrim(
                            rtrim(
                              ltrim(
                                substr(
                                  source__file__path,
                                  length(
                                    rtrim(
                                      rtrim(
                                        source__file__path,
                                        replace(source__file__path, :param_1, :param_2)
                                      ),
                                      :param_1
                                    )
                                  ) + :length_23
                                ),
                                :param_1
                              ),
                              replace(
                                ltrim(
                                  substr(
                                    source__file__path,
                                    length(
                                      rtrim(
                                        rtrim(
                                          source__file__path,
                                          replace(source__file__path, :param_1, :param_2)
                                        ),
                                        :param_1
                                      )
                                    ) + :length_24
                                  ),
                                  :param_1
                                ),
                                :param_1,
                                :param_2
                              )
                            ),
                            :param_1
                          )
                        ) + :length_21
                      ),
                      :param_1
                    )
                  ) - length(
                    rtrim(
                      ltrim(
                        substr(
                          ltrim(
                            substr(
                              source__file__path,
                              length(
                                rtrim(
                                  rtrim(
                                    source__file__path,
                                    replace(source__file__path, :param_1, :param_2)
                                  ),
                                  :param_1
                                )
                              ) + :length_25
                            ),
                            :param_1
                          ),
                          length(
                            rtrim(
                              rtrim(
                                ltrim(
                                  substr(
                                    source__file__path,
                                    length(
                                      rtrim(
                                        rtrim(
                                          source__file__path,
                                          replace(source__file__path, :param_1, :param_2)
                                        ),
                                        :param_1
                                      )
                                    ) + :length_26
                                  ),
                                  :param_1
                                ),
                                replace(
                                  ltrim(
                                    substr(
                                      source__file__path,
                                      length(
                                        rtrim(
                                          rtrim(
                                            source__file__path,
                                            replace(source__file__path, :param_1, :param_2)
                                          ),
                                          :param_1
                                        )
                                      ) + :length_27
                                    ),
                                    :param_1
                                  ),
                                  :param_1,
                                  :param_2
                                )
                              ),
                              :param_1
                            )
                          ) + :length_21
                        ),
                        :param_1
                      ),
                      replace(
                        ltrim(
                          substr(
                            ltrim(
                              substr(
                                source__file__path,
                                length(
                                  rtrim(
                                    rtrim(
                                      source__file__path,
                                      replace(source__file__path, :param_1, :param_2)
                                    ),
                                    :param_1
                                  )
                                ) + :length_28
                              ),
                              :param_1
                            ),
                            length(
                              rtrim(
                                rtrim(
                                  ltrim(
                                    substr(
                                      source__file__path,
                                      length(
                                        rtrim(
                                          rtrim(
                                            source__file__path,
                                            replace(source__file__path, :param_1, :param_2)
                                          ),
                                          :param_1
                                        )
                                      ) + :length_29
                                    ),
                                    :param_1
                                  ),
                                  replace(
                                    ltrim(
                                      substr(
                                        source__file__path,
                                        length(
                                          rtrim(
                                            rtrim(
                                              source__file__path,
                                              replace(source__file__path, :param_1, :param_2)
                                            ),
                                            :param_1
                                          )
                                        ) + :length_30
                                      ),
                                      :param_1
                                    ),
                                    :param_1,
                                    :param_2
                                  )
                                ),
                                :param_1
                              )
                            ) + :length_21
                          ),
                          :param_1
                        ),
                        :param_3,
                        :param_2
                      )
                    )
                  ) END
                ),
                :param_3
              ) AS stem

After PR

              CASE WHEN (
                instr(file__path, '/') = 0
              ) THEN rtrim(
                substr(
                  file__path, 
                  1, 
                  length(file__path) - CASE WHEN (
                    instr(file__path, '.') = 0
                  ) THEN 0 ELSE length(file__path) - length(
                    rtrim(
                      file__path, 
                      replace(file__path, '.', '')
                    )
                  ) END
                ), 
                '.'
              ) ELSE ltrim(
                rtrim(
                  substr(
                    file__path, 
                    length(
                      rtrim(
                        rtrim(
                          file__path, 
                          replace(file__path, '/', '')
                        ), 
                        '/'
                      )
                    ) + 1, 
                    (
                      length(file__path) - length(
                        rtrim(
                          rtrim(
                            file__path, 
                            replace(file__path, '/', '')
                          ), 
                          '/'
                        )
                      )
                    ) - CASE WHEN (
                      instr(
                        ltrim(
                          substr(
                            file__path, 
                            length(
                              rtrim(
                                rtrim(
                                  file__path, 
                                  replace(file__path, '/', '')
                                ), 
                                '/'
                              )
                            ) + 1
                          ), 
                          '/'
                        ), 
                        '.'
                      ) = 0
                    ) THEN 0 ELSE length(
                      ltrim(
                        substr(
                          file__path, 
                          length(
                            rtrim(
                              rtrim(
                                file__path, 
                                replace(file__path, '/', '')
                              ), 
                              '/'
                            )
                          ) + 1
                        ), 
                        '/'
                      )
                    ) - length(
                      rtrim(
                        ltrim(
                          substr(
                            file__path, 
                            length(
                              rtrim(
                                rtrim(
                                  file__path, 
                                  replace(file__path, '/', '')
                                ), 
                                '/'
                              )
                            ) + 1
                          ), 
                          '/'
                        ), 
                        replace(
                          ltrim(
                            substr(
                              file__path, 
                              length(
                                rtrim(
                                  rtrim(
                                    file__path, 
                                    replace(file__path, '/', '')
                                  ), 
                                  '/'
                                )
                              ) + 1
                            ), 
                            '/'
                          ), 
                          '.', 
                          ''
                        )
                      )
                    ) END
                  ), 
                  '.'
                ), 
                '/'
              ) END AS stem,

... as you can see the SQL is still hot garbage but there is less of it and the UX is slightly better

cloudflare-workers-and-pages · 2024-08-13T03:16:34Z

Deploying datachain-documentation with Cloudflare Pages

Latest commit:	`c95626d`
Status:	✅ Deploy successful!
Preview URL:	https://b2babfeb.datachain-documentation.pages.dev
Branch Preview URL:	https://full-path-file-stem.datachain-documentation.pages.dev

View logs

mattseddon · 2024-08-13T03:40:49Z

examples/get_started/common_sql_functions.py

-        stem=path.file_stem(path.name(C("file.path"))),
-        ext=path.file_ext(path.name(C("file.path"))),
+        stem=path.file_stem(C("file.path")),
+        ext=path.file_ext(C("file.path")),


[F] This accepted a full path already

mattseddon · 2024-08-13T06:08:34Z

Will need a studio companion PR.

dtulga

LGTM, thanks for adding this! Although, I would recommend only merging once the Studio/ClickHouse part is done and those tests pass as well.

mattseddon · 2024-08-14T01:57:14Z

LGTM, thanks for adding this! Although, I would recommend only merging once the Studio/ClickHouse part is done and those tests pass as well.

yeah, I'm a bit stuck. The companion is up (https://github.com/iterative/studio/pull/10469) but the Studio tests are completely borked (see #292 / #293). I'll wait til we get a decision or a proper fix.

dtulga · 2024-08-14T02:01:14Z

Yeah, that is less than ideal - if all the tests pass except those that were already broken, that's fine by me. (Although ideally these tests should be fixed on main at some point. 😕 And it may not be possible to release / update Studio, so that may not be an option either until the tests are fixed.)

dreadatour

Looks good to me! Thank you! 🙏

codecov · 2024-08-15T22:13:23Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.86%. Comparing base (58c63a4) to head (c95626d).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #284   +/-   ##
=======================================
  Coverage   86.85%   86.86%           
=======================================
  Files          90       90           
  Lines        9868     9874    +6     
  Branches     1995     1995           
=======================================
+ Hits         8571     8577    +6     
  Misses        947      947           
  Partials      350      350

Flag	Coverage Δ
datachain	`86.79% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mattseddon · 2024-08-16T05:26:18Z

blocked by https://github.com/iterative/studio/pull/10469 which is blocked by https://github.com/iterative/studio/pull/10480

mattseddon self-assigned this Aug 13, 2024

mattseddon marked this pull request as ready for review August 13, 2024 03:40

mattseddon requested a review from a team August 13, 2024 03:40

mattseddon commented Aug 13, 2024

View reviewed changes

dtulga approved these changes Aug 14, 2024

View reviewed changes

ilongin approved these changes Aug 14, 2024

View reviewed changes

dreadatour approved these changes Aug 15, 2024

View reviewed changes

mattseddon force-pushed the full-path-file-stem branch from c5d7295 to 0d18b13 Compare August 15, 2024 22:04

mattseddon force-pushed the full-path-file-stem branch 3 times, most recently from 6c3c65d to db0b454 Compare August 18, 2024 09:25

enable passing of a full path to file_stem

c95626d

mattseddon force-pushed the full-path-file-stem branch from db0b454 to c95626d Compare August 18, 2024 18:56

mattseddon merged commit 61aeed4 into main Aug 18, 2024
38 checks passed

mattseddon deleted the full-path-file-stem branch August 18, 2024 19:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

have file_stem accept a full path #284

have file_stem accept a full path #284

mattseddon commented Aug 13, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Aug 13, 2024 •

edited

Loading

mattseddon Aug 13, 2024

mattseddon commented Aug 13, 2024

dtulga left a comment

mattseddon commented Aug 14, 2024 •

edited

Loading

dtulga commented Aug 14, 2024 •

edited

Loading

dreadatour left a comment

codecov bot commented Aug 15, 2024 •

edited

Loading

mattseddon commented Aug 16, 2024

have file_stem accept a full path #284

have file_stem accept a full path #284

Conversation

mattseddon commented Aug 13, 2024 • edited Loading

Before PR

After PR

cloudflare-workers-and-pages bot commented Aug 13, 2024 • edited Loading

Deploying datachain-documentation with Cloudflare Pages

mattseddon Aug 13, 2024

Choose a reason for hiding this comment

mattseddon commented Aug 13, 2024

dtulga left a comment

Choose a reason for hiding this comment

mattseddon commented Aug 14, 2024 • edited Loading

dtulga commented Aug 14, 2024 • edited Loading

dreadatour left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 15, 2024 • edited Loading

Codecov Report

mattseddon commented Aug 16, 2024

mattseddon commented Aug 13, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Aug 13, 2024 •

edited

Loading

mattseddon commented Aug 14, 2024 •

edited

Loading

dtulga commented Aug 14, 2024 •

edited

Loading

codecov bot commented Aug 15, 2024 •

edited

Loading