Skip to content

Conversation

@alamb
Copy link
Contributor

@alamb alamb commented Sep 25, 2024

Which issue does this PR close?

Follow on to #12395 from @Rachelint

Rationale for this change

Part of reviewing https://github.com/apache/datafusion/pull/12395I spent some non trivial time reading this code, and I wanted to encodde my learnings as comments

What changes are included in this PR?

  1. Add comments to various functions
  2. Update parameter names

Are these changes tested?

No code, just docs and name changes

Are there any user-facing changes?

No, this is all internal code

@alamb alamb marked this pull request as draft September 25, 2024 20:03
@github-actions github-actions bot added physical-expr Changes to the physical-expr crates functions Changes to functions implementation labels Sep 25, 2024
@alamb alamb force-pushed the alamb/more_trim_docs branch from 0e211b9 to 38210c9 Compare September 25, 2024 20:55
@alamb alamb marked this pull request as ready for review September 25, 2024 20:55
/// raw must be a valid view
/// substr must be a valid substring of raw
/// # Safety
/// original_view must be a valid view
Copy link
Contributor

Choose a reason for hiding this comment

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

what is valid view? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call -- I tried to clarify in c8668df

Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

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

lgtm thanks @alamb

@github-actions github-actions bot removed the physical-expr Changes to the physical-expr crates label Sep 26, 2024
@alamb alamb merged commit 79d40c4 into apache:main Sep 27, 2024
@alamb
Copy link
Contributor Author

alamb commented Sep 27, 2024

Thanks again @comphead

@alamb alamb deleted the alamb/more_trim_docs branch September 27, 2024 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

functions Changes to functions implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants