-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
GH-2930: Add issue templates #2931
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding this!
attributes: | ||
label: Component(s) | ||
multiple: true | ||
options: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually need these options
? Or should it be consistent with modules like parquet-avro
, parquet-cli
, etc.? Same question for other yamls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also please note I introduced as many new labels as I could during migration. We now might want to reduce the current set by deleting them here (delete is irreversible). Same goes for milestones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I would not add the modules as is. It does not make sense to differentiate e.g.
parquet-common
,parquet-column
, orparquet-hadoop
(etc.). These are all parts of the "core". - It might make sense to mention the "bindings" separately (
parquet-arrow
,parquet-avro
,parquet-pig
,parquet-protobuf
,parquet-scala
,parquet-thrift
). - If we want to list components, Java is too wide.
- I think it makes more sense to somehow sum up Release+CI+Packaging.
- What is Documentation? Adding javadocs? I would add these changes to the related module instead.
- I would add parquet-cli as a separate item
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback @gszadovszky. I've change the list like so:
options:
- - Java
- Release
- - Documentation
- Continuous Integration
- - Packaging
+ - Arrow
+ - Avro
+ - Pig
+ - Protobuf
+ - Scala
+ - Thrift
+ - CLI
- Benchmarking
- Other
I think it makes more sense to somehow sum up Release+CI+Packaging.
Agreed, but I don't have a good suggestion how. For now I just removed packaging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @rok!
It would be nice to see somehow how many issues we have related to CI
and Release
. Maybe Other
is enough but not sure. I am fine as is, though.
One thing is clearly missing. This is the core functionality that most PRs have updates in. These are the modules parquet-column
, parquet-common
, parquet-encoding
, parquet-format-structures
, parquet-generator
, parquet-hadoop
. I am fine calling it Core
if there are no better suggestion.
parquet-hadoop
is definitely part of core currently, even though we want to get rid of the hadoop dependencies. After this work would be done, the separate component Hadoop
would make sense, but currently I don't think we need it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @rok for working on this 🙌 I would be inclined to remove Other
and make it optional. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback! :D I've pushed a change and here's the current list:
- Core
- Release
- Continuous Integration
- Arrow
- Avro
- Pig
- Protobuf
- Scala
- Thrift
- CLI
- Benchmarking
I'm glad to defer to your suggestions as I'm quite unfamiliar with parquet-java
(except for the migration).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more thing, I was looking at Iceberg regarding @gszadovszky's comment.
I think it makes more sense to somehow sum up Release+CI+Packaging.
At Iceberg we've packaged these into a Build
label.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a good idea. Will do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @rok for working on this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1. Thanks @rok!
Co-authored-by: Gang Wu <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @rok!
I will merge this after next 24 hours if no objection. |
This introduces Arrow-like issue templates to structure and categorize issues somewhat as they are reported.
See #2930