martin-g commented on code in PR #3624:
URL: https://github.com/apache/avro/pull/3624#discussion_r2689192481
##########
lang/php/lib/Schema/AvroField.php:
##########
@@ -82,35 +84,86 @@ class AvroField extends AvroSchema implements
AvroAliasedSchema
*/
private ?string $order;
private ?array $aliases;
+ private ?string $doc;
/**
- * @throws AvroSchemaParseException
+ * @param array<string> $aliases
* @todo Check validity of $default value
*/
- public function __construct(
- ?string $name,
+ private function __construct(
Review Comment:
Why private ?
##########
lang/php/lib/Schema/AvroField.php:
##########
@@ -82,35 +84,86 @@ class AvroField extends AvroSchema implements
AvroAliasedSchema
*/
private ?string $order;
private ?array $aliases;
+ private ?string $doc;
/**
- * @throws AvroSchemaParseException
+ * @param array<string> $aliases
* @todo Check validity of $default value
*/
- public function __construct(
- ?string $name,
+ private function __construct(
+ string $name,
string|AvroSchema $schema,
bool $isTypeFromSchemata,
bool $hasDefault,
mixed $default,
?string $order = null,
- mixed $aliases = null
+ ?array $aliases = null,
+ ?string $doc = null
) {
- if (!AvroName::isWellFormedName($name)) {
Review Comment:
This is no more needed because now the `$name` parameter has a non-null type
?! (a `string`)
##########
lang/php/lib/Schema/AvroField.php:
##########
@@ -82,35 +84,86 @@ class AvroField extends AvroSchema implements
AvroAliasedSchema
*/
private ?string $order;
private ?array $aliases;
+ private ?string $doc;
/**
- * @throws AvroSchemaParseException
+ * @param array<string> $aliases
* @todo Check validity of $default value
*/
- public function __construct(
- ?string $name,
+ private function __construct(
+ string $name,
string|AvroSchema $schema,
bool $isTypeFromSchemata,
bool $hasDefault,
mixed $default,
?string $order = null,
- mixed $aliases = null
+ ?array $aliases = null,
+ ?string $doc = null
) {
- if (!AvroName::isWellFormedName($name)) {
- throw new AvroSchemaParseException('Field requires a "name"
attribute');
- }
-
parent::__construct($schema);
$this->name = $name;
$this->isTypeFromSchemata = $isTypeFromSchemata;
$this->hasDefault = $hasDefault;
if ($this->hasDefault) {
$this->default = $default;
}
- self::checkOrderValue($order);
$this->order = $order;
- self::hasValidAliases($aliases);
$this->aliases = $aliases;
+
+ $this->doc = $doc;
+ }
+
+ /**
+ * @throws AvroSchemaParseException
+ */
+ public static function fromFieldDefinition(array $avro, ?string
$defaultNamespace, AvroNamedSchemata $schemata): self
+ {
+ $name = $avro[self::FIELD_NAME_ATTR] ?? null;
+ $type = $avro[AvroSchema::TYPE_ATTR] ?? null;
+ $order = $avro[self::ORDER_ATTR] ?? null;
+ $aliases = $avro[AvroSchema::ALIASES_ATTR] ?? null;
+ $doc = $avro[AvroSchema::DOC_ATTR] ?? null;
+
+ if (!AvroName::isWellFormedName($name)) {
Review Comment:
OK! I see - it is just moved here.
##########
lang/php/lib/Schema/AvroSchema.php:
##########
@@ -544,6 +543,17 @@ public static function hasValidAliases($aliases): void
}
}
+ public static function hasValidDoc(mixed $doc): void
+ {
+ if (is_string($doc) || is_null($doc)) {
+ return;
+ }
+
+ throw new AvroSchemaParseException(
+ 'Invalid doc value. Must be a string or empty.'
Review Comment:
Agreed!
##########
lang/php/lib/Schema/AvroField.php:
##########
@@ -131,13 +184,21 @@ public function toAvro(): string|array
$avro[self::ORDER_ATTR] = $this->order;
}
+ if (!is_null($this->aliases)) {
+ $avro[AvroSchema::ALIASES_ATTR] = $this->aliases;
+ }
+
+ if (!is_null($this->doc)) {
+ $avro[AvroSchema::DOC_ATTR] = $this->doc;
+ }
+
return $avro;
}
/**
* @returns string the name of this field
*/
- public function name()
+ public function name(): ?string
Review Comment:
@mattiabasone WDYT about this ? The suggestion looks good to me
##########
lang/php/lib/Schema/AvroArraySchema.php:
##########
@@ -27,42 +27,35 @@
class AvroArraySchema extends AvroSchema
{
/**
- * @var AvroName|AvroSchema named schema name or AvroSchema of
- * array element
+ * @var AvroSchema AvroSchema of array element
Review Comment:
```suggestion
* @var AvroSchema The schema of the array elements
```
##########
lang/php/lib/Schema/AvroRecordSchema.php:
##########
@@ -62,62 +62,30 @@ public function __construct(
* @throws AvroSchemaParseException
*/
public static function parseFields(
- array $field_data,
+ array $fieldsDefinitions,
?string $default_namespace,
- ?AvroNamedSchemata $schemata = null
+ AvroNamedSchemata $schemata
): array {
$fields = [];
- $field_names = [];
- $alias_names = [];
- foreach ($field_data as $field) {
- $name = $field[AvroField::FIELD_NAME_ATTR] ?? null;
- $type = $field[AvroSchema::TYPE_ATTR] ?? null;
- $order = $field[AvroField::ORDER_ATTR] ?? null;
- $aliases = $field[AvroSchema::ALIASES_ATTR] ?? null;
-
- $default = null;
- $has_default = false;
- if (array_key_exists(AvroField::DEFAULT_ATTR, $field)) {
- $default = $field[AvroField::DEFAULT_ATTR];
- $has_default = true;
- }
+ $fieldNames = [];
+ $aliasNames = [];
+ foreach ($fieldsDefinitions as $fieldsDefinition) {
+ $name = $fieldsDefinition[AvroField::FIELD_NAME_ATTR] ?? null;
- if (in_array($name, $field_names)) {
+ if (in_array($name, $fieldNames)) {
throw new AvroSchemaParseException(
sprintf("Field name %s is already in use", $name)
);
}
- $is_schema_from_schemata = false;
- $field_schema = null;
- if (
- is_string($type)
- && $field_schema = $schemata->schemaByName(
- new AvroName($type, null, $default_namespace)
- )
- ) {
- $is_schema_from_schemata = true;
- } elseif (is_string($type) && self::isPrimitiveType($type)) {
- $field_schema = self::subparse($field, $default_namespace,
$schemata);
- } else {
- $field_schema = self::subparse($type, $default_namespace,
$schemata);
- }
+ $new_field = AvroField::fromFieldDefinition($fieldsDefinition,
$default_namespace, $schemata);
Review Comment:
Agreed!
##########
lang/php/lib/Schema/AvroNamedSchemata.php:
##########
@@ -62,7 +62,7 @@ public function schema(string $fullname): ?AvroSchema
* @param AvroNamedSchema $schema schema to add to this existing schemata
* @throws AvroSchemaParseException
*/
- public function cloneWithNewSchema(AvroNamedSchema $schema):
AvroNamedSchemata
+ public function registerNamedSchema(AvroNamedSchema $schema): self
Review Comment:
This is an API break.
If you want to rename it then you have to keep a deprecated forwarder for
1.13.x and you can remove it for 1.14.0
##########
lang/php/test/SchemaTest.php:
##########
@@ -28,15 +30,15 @@
class SchemaExample
{
- public $name;
- public $normalized_schema_string;
+ public string $name;
+ public string $normalized_schema_string;
Review Comment:
Why this is still snake_case ? You renamed several properties above to
camelCase
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
