Skip to content

Conversation

@AnzhiZhang
Copy link
Contributor

@AnzhiZhang AnzhiZhang commented Aug 14, 2025

There is an example Python UDF generator referenced from the wiki. However, the example returns a list, which is a binary attribute type. Intuitively, a starter may think the attribute has an integer type, which will cause some confusion.

#3657 updated that example to return integers. From our discussion today, we will remain with the previous binary example and add an integer example for starters.

The name of the binary generator example is updated. Wiki link here needs to be updated, or we can just leave the binary example for the guide.

Resolves #3657

Copilot AI review requested due to automatic review settings August 14, 2025 22:29
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds generator operator examples for binary and integer data types, refactoring the existing generator operator to be more specific. The changes include renaming the original generator to focus on integer generation and creating a new binary generator operator with corresponding test files.

  • Renamed existing generator operator to GeneratorOperatorInteger with updated documentation
  • Added new GeneratorOperatorBinary class that produces binary data
  • Created comprehensive test files for both operator types

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
generator_operator_integer.py Renamed class and added docstring for integer-focused generator
generator_operator_binary.py New binary generator operator implementation
test_generator_operator_integer.py Updated test file with renamed imports and methods
test_generator_operator_binary.py New test file for binary generator operator

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@Yicong-Huang
Copy link
Contributor

@AnzhiZhang thanks. could you please remove the @ from the first comment (or called PR description)? we use that space for describing the PR changes, and we tag other people using comments after.

Copy link
Contributor

@Yicong-Huang Yicong-Huang left a comment

Choose a reason for hiding this comment

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

LGTM. please go ahead update the wiki to use the integer example.

@AnzhiZhang
Copy link
Contributor Author

AnzhiZhang commented Aug 14, 2025

@AnzhiZhang thanks. could you please remove the @ from the first comment (or called PR description)? we use that space for describing the PR changes, and we tag other people using comments after.

Done. Is the current text look good? Or I can rephrase it more like a description.

@Yicong-Huang
Copy link
Contributor

also, please change "related #issue number" to "closes" or "fixes"

@Yicong-Huang
Copy link
Contributor

Yicong-Huang commented Aug 14, 2025

@AnzhiZhang thanks. could you please remove the @ from the first comment (or called PR description)? we use that space for describing the PR changes, and we tag other people using comments after.

Done. Is the current text look good? Or I can rephrase it more like a description.

it will be good to update the reason for the change. our discussion should be reflected here so other readers have context.

@AnzhiZhang
Copy link
Contributor Author

also, please change "related #issue number" to "closes" or "fixes"

It's a related PR, I will use "resolves" instead.

@AnzhiZhang thanks. could you please remove the @ from the first comment (or called PR description)? we use that space for describing the PR changes, and we tag other people using comments after.

Done. Is the current text look good? Or I can rephrase it more like a description.

it will be good to update the reason for the change. our discussion should be reflected here so other readers have context.

Updated, please have a look and let me know if any further changes are required.

@Yicong-Huang
Copy link
Contributor

LGTM

@aglinxinyuan
Copy link
Contributor

Add "example" in the title? It's different from the operators in the system.

@AnzhiZhang AnzhiZhang changed the title feat: generator operators for binary and integer feat: generator operator examples for binary and integer Aug 19, 2025
@AnzhiZhang
Copy link
Contributor Author

Add "example" in the title? It's different from the operators in the system.

done

@aglinxinyuan aglinxinyuan enabled auto-merge (squash) August 20, 2025 00:01
@aglinxinyuan aglinxinyuan disabled auto-merge August 20, 2025 04:04
@aglinxinyuan aglinxinyuan merged commit e2c64c5 into apache:main Aug 24, 2025
17 of 18 checks passed
@AnzhiZhang AnzhiZhang deleted the examples branch August 24, 2025 23:06
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.

3 participants