You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/09/29 03:49:52 UTC
[GitHub] [pulsar] merlimat opened a new pull request #12233: [Python] Allow required=False default=None setting on fields
merlimat opened a new pull request #12233:
URL: https://github.com/apache/pulsar/pull/12233
### Motivation
Recently, a new keyword `required_default` was added to address the case where a `default=None` needs to be set on the schema. The problem with this argument is that it's really counterintuitive on its behavior.
The problem was that we were not able to distinguish between `default=None` as in "there's no default" and `default=None` as in "there is a default which value is None".
The `required_default=True` was introduced for that but it's very confusing.
Instead, we should be able to just be able to use the original `required` and `default` keywords.
### Modification
Use a special object value to distinguish between a default not set and default=None scenarios.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #12233: [Python] Allow required=False default=None setting on fields
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12233:
URL: https://github.com/apache/pulsar/pull/12233#issuecomment-974749323
@merlimat:Thanks for your contribution. For this PR, do we need to update docs?
(The [PR template contains info about doc](https://github.com/apache/pulsar/blob/master/.github/PULL_REQUEST_TEMPLATE.md#documentation), which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] 315157973 commented on pull request #12233: [Python] Allow required=False default=None setting on fields
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #12233:
URL: https://github.com/apache/pulsar/pull/12233#issuecomment-957054506
@merlimat PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] congbobo184 commented on a change in pull request #12233: [Python] Allow required=False default=None setting on fields
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on a change in pull request #12233:
URL: https://github.com/apache/pulsar/pull/12233#discussion_r718156320
##########
File path: pulsar-client-cpp/python/pulsar/schema/definition.py
##########
@@ -114,34 +121,50 @@ def schema_info(cls, defined_names):
defined_names.add(namespace_name)
- schema = {'name': str(cls.__name__)}
+ schema = {
+ 'type': 'record',
+ 'name': str(cls.__name__)
+ }
if cls._avro_namespace is not None:
schema['namespace'] = cls._avro_namespace
- schema['type'] = 'record'
schema['fields'] = []
- for name in sorted(cls._fields.keys()):
+ if cls._sorted_fields:
+ fields = sorted(cls._fields.keys())
+ else:
+ fields = cls._fields.keys()
+ for name in fields:
field = cls._fields[name]
field_type = field.schema_info(defined_names) \
if field._required else ['null', field.schema_info(defined_names)]
- schema['fields'].append({
- 'name': name,
- 'type': field_type,
- 'default': field.default()
- }) if field.required_default() else schema['fields'].append({
- 'name': name,
- 'type': field_type,
- })
+
+ if field._required_default:
+ schema['fields'].append({
+ 'name': name,
+ 'default': None,
+ 'type': field_type
+ })
+ elif not field._required and field.default() != _empty_default:
Review comment:
why we need to jude field._required
##########
File path: pulsar-client-cpp/python/pulsar/schema/definition.py
##########
@@ -114,34 +121,50 @@ def schema_info(cls, defined_names):
defined_names.add(namespace_name)
- schema = {'name': str(cls.__name__)}
+ schema = {
+ 'type': 'record',
+ 'name': str(cls.__name__)
+ }
if cls._avro_namespace is not None:
schema['namespace'] = cls._avro_namespace
- schema['type'] = 'record'
schema['fields'] = []
- for name in sorted(cls._fields.keys()):
+ if cls._sorted_fields:
+ fields = sorted(cls._fields.keys())
+ else:
+ fields = cls._fields.keys()
+ for name in fields:
field = cls._fields[name]
field_type = field.schema_info(defined_names) \
if field._required else ['null', field.schema_info(defined_names)]
- schema['fields'].append({
- 'name': name,
- 'type': field_type,
- 'default': field.default()
- }) if field.required_default() else schema['fields'].append({
- 'name': name,
- 'type': field_type,
- })
+
+ if field._required_default:
+ schema['fields'].append({
+ 'name': name,
+ 'default': None,
Review comment:
if `field.default()=_empty_default` the `'default': None`, if `field.default()!`=`_empty_default` the `default': field.default()`
##########
File path: pulsar-client-cpp/python/pulsar/schema/definition.py
##########
@@ -114,34 +121,50 @@ def schema_info(cls, defined_names):
defined_names.add(namespace_name)
- schema = {'name': str(cls.__name__)}
+ schema = {
+ 'type': 'record',
+ 'name': str(cls.__name__)
+ }
if cls._avro_namespace is not None:
schema['namespace'] = cls._avro_namespace
- schema['type'] = 'record'
schema['fields'] = []
- for name in sorted(cls._fields.keys()):
+ if cls._sorted_fields:
+ fields = sorted(cls._fields.keys())
+ else:
+ fields = cls._fields.keys()
+ for name in fields:
field = cls._fields[name]
field_type = field.schema_info(defined_names) \
if field._required else ['null', field.schema_info(defined_names)]
- schema['fields'].append({
- 'name': name,
- 'type': field_type,
- 'default': field.default()
- }) if field.required_default() else schema['fields'].append({
- 'name': name,
- 'type': field_type,
- })
+
+ if field._required_default:
+ schema['fields'].append({
+ 'name': name,
+ 'default': None,
Review comment:
if `field.default()=_empty_default` the `'default': None`, if `field.default()!`=`_empty_default` the `default: field.default()`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #12233: [Python] Allow required=False default=None setting on fields
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #12233:
URL: https://github.com/apache/pulsar/pull/12233#issuecomment-1036673960
Removing the 2.8.3 label since this is an enhancement.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Anonymitaet commented on pull request #12233: [Python] Allow required=False default=None setting on fields
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on pull request #12233:
URL: https://github.com/apache/pulsar/pull/12233#issuecomment-977662164
Hi @merlimat seems that this is an internal implementation which does not affect docs?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Anonymitaet commented on pull request #12233: [Python] Allow required=False default=None setting on fields
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on pull request #12233:
URL: https://github.com/apache/pulsar/pull/12233#issuecomment-930939539
Thanks for your contribution. For this PR, do we need to update docs?
(The [PR template contains info about doc](https://github.com/apache/pulsar/blob/master/.github/PULL_REQUEST_TEMPLATE.md#documentation), which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] congbobo184 commented on a change in pull request #12233: [Python] Allow required=False default=None setting on fields
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on a change in pull request #12233:
URL: https://github.com/apache/pulsar/pull/12233#discussion_r718156320
##########
File path: pulsar-client-cpp/python/pulsar/schema/definition.py
##########
@@ -114,34 +121,50 @@ def schema_info(cls, defined_names):
defined_names.add(namespace_name)
- schema = {'name': str(cls.__name__)}
+ schema = {
+ 'type': 'record',
+ 'name': str(cls.__name__)
+ }
if cls._avro_namespace is not None:
schema['namespace'] = cls._avro_namespace
- schema['type'] = 'record'
schema['fields'] = []
- for name in sorted(cls._fields.keys()):
+ if cls._sorted_fields:
+ fields = sorted(cls._fields.keys())
+ else:
+ fields = cls._fields.keys()
+ for name in fields:
field = cls._fields[name]
field_type = field.schema_info(defined_names) \
if field._required else ['null', field.schema_info(defined_names)]
- schema['fields'].append({
- 'name': name,
- 'type': field_type,
- 'default': field.default()
- }) if field.required_default() else schema['fields'].append({
- 'name': name,
- 'type': field_type,
- })
+
+ if field._required_default:
+ schema['fields'].append({
+ 'name': name,
+ 'default': None,
+ 'type': field_type
+ })
+ elif not field._required and field.default() != _empty_default:
Review comment:
why we need to jude field._required
##########
File path: pulsar-client-cpp/python/pulsar/schema/definition.py
##########
@@ -114,34 +121,50 @@ def schema_info(cls, defined_names):
defined_names.add(namespace_name)
- schema = {'name': str(cls.__name__)}
+ schema = {
+ 'type': 'record',
+ 'name': str(cls.__name__)
+ }
if cls._avro_namespace is not None:
schema['namespace'] = cls._avro_namespace
- schema['type'] = 'record'
schema['fields'] = []
- for name in sorted(cls._fields.keys()):
+ if cls._sorted_fields:
+ fields = sorted(cls._fields.keys())
+ else:
+ fields = cls._fields.keys()
+ for name in fields:
field = cls._fields[name]
field_type = field.schema_info(defined_names) \
if field._required else ['null', field.schema_info(defined_names)]
- schema['fields'].append({
- 'name': name,
- 'type': field_type,
- 'default': field.default()
- }) if field.required_default() else schema['fields'].append({
- 'name': name,
- 'type': field_type,
- })
+
+ if field._required_default:
+ schema['fields'].append({
+ 'name': name,
+ 'default': None,
Review comment:
if `field.default()=_empty_default` the `'default': None`, if `field.default()!`=`_empty_default` the `default': field.default()`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] congbobo184 commented on a change in pull request #12233: [Python] Allow required=False default=None setting on fields
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on a change in pull request #12233:
URL: https://github.com/apache/pulsar/pull/12233#discussion_r718157326
##########
File path: pulsar-client-cpp/python/pulsar/schema/definition.py
##########
@@ -114,34 +121,50 @@ def schema_info(cls, defined_names):
defined_names.add(namespace_name)
- schema = {'name': str(cls.__name__)}
+ schema = {
+ 'type': 'record',
+ 'name': str(cls.__name__)
+ }
if cls._avro_namespace is not None:
schema['namespace'] = cls._avro_namespace
- schema['type'] = 'record'
schema['fields'] = []
- for name in sorted(cls._fields.keys()):
+ if cls._sorted_fields:
+ fields = sorted(cls._fields.keys())
+ else:
+ fields = cls._fields.keys()
+ for name in fields:
field = cls._fields[name]
field_type = field.schema_info(defined_names) \
if field._required else ['null', field.schema_info(defined_names)]
- schema['fields'].append({
- 'name': name,
- 'type': field_type,
- 'default': field.default()
- }) if field.required_default() else schema['fields'].append({
- 'name': name,
- 'type': field_type,
- })
+
+ if field._required_default:
+ schema['fields'].append({
+ 'name': name,
+ 'default': None,
Review comment:
if `field.default()=_empty_default` the `'default': None`, if `field.default()!`=`_empty_default` the `default: field.default()`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a change in pull request #12233: [Python] Allow required=False default=None setting on fields
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #12233:
URL: https://github.com/apache/pulsar/pull/12233#discussion_r718135581
##########
File path: pulsar-client-cpp/python/pulsar/schema/definition.py
##########
@@ -60,8 +64,10 @@ class Record(with_metaclass(RecordMeta, object)):
# This field is used to set namespace for Avro Record schema.
_avro_namespace = None
- def __init__(self, default=None, required_default=False, required=False, *args, **kwargs):
Review comment:
Shall we need to keep the old configuration `required_default=False` for compatibility consideration?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] gaoran10 commented on a change in pull request #12233: [Python] Allow required=False default=None setting on fields
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on a change in pull request #12233:
URL: https://github.com/apache/pulsar/pull/12233#discussion_r718150336
##########
File path: pulsar-client-cpp/python/pulsar/schema/definition.py
##########
@@ -60,10 +64,13 @@ class Record(with_metaclass(RecordMeta, object)):
# This field is used to set namespace for Avro Record schema.
_avro_namespace = None
- def __init__(self, default=None, required_default=False, required=False, *args, **kwargs):
- self._required_default = required_default
+ # Generate a schema where fields are sorted alphabetically
+ _sorted_fields = False
+
+ def __init__(self, default=None, required=False, required_default=False, *args, **kwargs):
Review comment:
Do we need to change the param `default` of the `Record` to `_empty_default`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] 315157973 commented on pull request #12233: [Python] Allow required=False default=None setting on fields
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #12233:
URL: https://github.com/apache/pulsar/pull/12233#issuecomment-957054506
@merlimat PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a change in pull request #12233: [Python] Allow required=False default=None setting on fields
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #12233:
URL: https://github.com/apache/pulsar/pull/12233#discussion_r718135581
##########
File path: pulsar-client-cpp/python/pulsar/schema/definition.py
##########
@@ -60,8 +64,10 @@ class Record(with_metaclass(RecordMeta, object)):
# This field is used to set namespace for Avro Record schema.
_avro_namespace = None
- def __init__(self, default=None, required_default=False, required=False, *args, **kwargs):
Review comment:
Shall we need to keep the old configuration `required_default=False` for compatibility consideration?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat commented on a change in pull request #12233: [Python] Allow required=False default=None setting on fields
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #12233:
URL: https://github.com/apache/pulsar/pull/12233#discussion_r718141453
##########
File path: pulsar-client-cpp/python/pulsar/schema/definition.py
##########
@@ -60,8 +64,10 @@ class Record(with_metaclass(RecordMeta, object)):
# This field is used to set namespace for Avro Record schema.
_avro_namespace = None
- def __init__(self, default=None, required_default=False, required=False, *args, **kwargs):
Review comment:
I put it back for compatibility
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #12233: [Python] Allow required=False default=None setting on fields
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12233:
URL: https://github.com/apache/pulsar/pull/12233#issuecomment-974749323
@merlimat:Thanks for your contribution. For this PR, do we need to update docs?
(The [PR template contains info about doc](https://github.com/apache/pulsar/blob/master/.github/PULL_REQUEST_TEMPLATE.md#documentation), which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #12233: [Python] Allow required=False default=None setting on fields
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12233:
URL: https://github.com/apache/pulsar/pull/12233#issuecomment-1066259999
The pr had no activity for 30 days, mark with Stale label.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] gaoran10 commented on a change in pull request #12233: [Python] Allow required=False default=None setting on fields
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on a change in pull request #12233:
URL: https://github.com/apache/pulsar/pull/12233#discussion_r718150336
##########
File path: pulsar-client-cpp/python/pulsar/schema/definition.py
##########
@@ -60,10 +64,13 @@ class Record(with_metaclass(RecordMeta, object)):
# This field is used to set namespace for Avro Record schema.
_avro_namespace = None
- def __init__(self, default=None, required_default=False, required=False, *args, **kwargs):
- self._required_default = required_default
+ # Generate a schema where fields are sorted alphabetically
+ _sorted_fields = False
+
+ def __init__(self, default=None, required=False, required_default=False, *args, **kwargs):
Review comment:
Do we need to change the param `default` of the `Record` to `_empty_default`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat commented on a change in pull request #12233: [Python] Allow required=False default=None setting on fields
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #12233:
URL: https://github.com/apache/pulsar/pull/12233#discussion_r718141453
##########
File path: pulsar-client-cpp/python/pulsar/schema/definition.py
##########
@@ -60,8 +64,10 @@ class Record(with_metaclass(RecordMeta, object)):
# This field is used to set namespace for Avro Record schema.
_avro_namespace = None
- def __init__(self, default=None, required_default=False, required=False, *args, **kwargs):
Review comment:
I put it back for compatibility
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] 315157973 commented on pull request #12233: [Python] Allow required=False default=None setting on fields
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #12233:
URL: https://github.com/apache/pulsar/pull/12233#issuecomment-957054506
@merlimat PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org