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.

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

Update docs for some operators #585

Merged
merged 4 commits into from
Oct 18, 2024
Merged

Update docs for some operators #585

merged 4 commits into from
Oct 18, 2024

Conversation

satrana42
Copy link
Contributor

@satrana42 satrana42 commented Oct 17, 2024

In this change, we update or add documentation for operator
changes done in #553, #556, #535, #560 and #578.


Important

Update documentation for firstk, max, min, changelog, and join operators with new parameters and type clarifications.

  • Aggregations:
    • firstk.md: Add dropnull parameter to drop None values.
    • max.md and min.md: Update of and into_field types to include date and datetime.
  • Operators:
    • changelog.md: New file added for Changelog operator documentation.
    • join.md: Add fields parameter to specify right dataset fields in output.

This description was created by Ellipsis for b601ac1. It will automatically update as commits are pushed.

@satrana42 satrana42 self-assigned this Oct 17, 2024
Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 2f9b0b0 in 24 seconds

More details
  • Looked at 218 lines of code in 6 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. docs/pages/api-reference/aggregations/max.md:27
  • Draft comment:
    The default parameter should include date and datetime in its type to match the of parameter.
<Expandable title="default" type="Union[int, float, date, datetime]">
  • Reason this comment was not posted:
    Comment was on unchanged code.
2. docs/pages/api-reference/aggregations/min.md:27
  • Draft comment:
    The default parameter should include date and datetime in its type to match the of parameter.
<Expandable title="default" type="Union[int, float, date, datetime]">
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_A54YuJp5lUxNYlHT


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Comment on lines 7 to 10
Operator to convert a keyed dataset into a keyless one, where the underlying delta
frame of the dataset is presented as a changelog. All key fields are converted into
normal fields, and an additional column is added which contains the kind (insert
or delete) of the delta.
Copy link
Contributor

Choose a reason for hiding this comment

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

users dont know about delta frame ( an internal detail ),

Operator to convert a keyed dataset into a CDC changelog stream. All key fields are converted into value fields, and an additional column is added, indicating the type of change (insert or delete) for the delta.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Python users don't know about value fields either

Copy link
Contributor

Choose a reason for hiding this comment

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

they do

#### Returns

<Expandable type="Dataset">
Returns a dataset with underlying delta frame of the input dataset is presented as
Copy link
Contributor

Choose a reason for hiding this comment

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

fix this accordingly

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Incremental review on 69a57d7 in 15 seconds

More details
  • Looked at 58 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_F2vOl02fnkiVRuxY


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

<Expandable type="Dataset">
Returns a dataset with input keyed dataset into an append only CDC changelog
stream. All key fields converted into normal fields, and an additional
column is addedm which contains the type of change (insert or delete) for the
Copy link

Choose a reason for hiding this comment

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

Typo: Replace 'addedm' with 'added'.

Suggested change
column is addedm which contains the type of change (insert or delete) for the
column is added, which contains the type of change (insert or delete) for the

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on dc5162a in 9 seconds

More details
  • Looked at 13 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. docs/pages/api-reference/operators/changelog.md:28
  • Draft comment:
    Consider rephrasing for clarity: "Returns a dataset with the input keyed dataset converted into an append-only CDC changelog stream. All key fields are converted into normal fields, and an additional column is added, which contains the type of change (insert or delete) for the delta."
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The documentation is mostly clear, but there are some minor grammatical issues and a potential for confusion in the description of the parameters.

Workflow ID: wflow_3AVyuZmxbEA21mMj


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@satrana42 satrana42 enabled auto-merge (squash) October 18, 2024 07:06
Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on d638ccb in 7 seconds

More details
  • Looked at 13 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. docs/pages/api-reference/operators/changelog.md:28
  • Draft comment:
    Minor grammatical error. Consider rephrasing for clarity.
  • Reason this comment was not posted:
    Confidence changes required: 10%
    The documentation for the changelog operator is mostly clear, but there is a minor grammatical error in the description of the Returns section.

Workflow ID: wflow_nFmIwBcR7bAF42Bn


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on b601ac1 in 15 seconds

More details
  • Looked at 12 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. .wordlist.txt:23
  • Draft comment:
    The addition of 'changelog' seems redundant as 'Changelog' is already present. Consider removing one to maintain consistency.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The wordlist includes both capitalized and lowercase versions of words, suggesting that this might be intentional to account for case sensitivity in different contexts. The comment points out a potential redundancy, but without knowing the specific use case for the wordlist, it's speculative to assume that one should be removed. The presence of both forms might be necessary for the application's functionality.
    I might be missing the specific context or requirements for the wordlist, which could justify having both 'Changelog' and 'changelog'. The comment assumes redundancy without strong evidence that it is an issue.
    Even without specific context, the pattern in the wordlist suggests that both forms are intentionally included. The comment lacks strong evidence that this is an issue that needs addressing.
    The comment should be deleted because it speculates on redundancy without strong evidence, and the pattern in the wordlist suggests intentional inclusion of both forms.

Workflow ID: wflow_eNbe93k0yNm3JUGL


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@satrana42 satrana42 merged commit 3da51ef into main Oct 18, 2024
8 checks passed
@satrana42 satrana42 deleted the sat/update_docs branch October 18, 2024 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants