martin-g commented on code in PR #3526:
URL: https://github.com/apache/avro/pull/3526#discussion_r2454001133
##########
lang/php/lib/Datum/AvroIOBinaryEncoder.php:
##########
@@ -187,4 +188,46 @@ public function writeBytes($bytes)
$this->writeLong(strlen($bytes));
$this->write($bytes);
}
+
+ public function writeDecimal($decimal, int $scale, int $precision): void
+ {
+ if (!is_numeric($decimal)) {
+ throw new AvroException('Decimal must be a numeric value');
Review Comment:
You may want to print the problematic value for easier debugging in such
exception messages
##########
lang/php/lib/Schema/AvroLogicalType.php:
##########
@@ -0,0 +1,112 @@
+<?php
+
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+namespace Apache\Avro\Schema;
+
+class AvroLogicalType
+{
+ public const ATTRIBUTE_DECIMAL_PRECISION = 'precision';
+ public const ATTRIBUTE_DECIMAL_SCALE = 'scale';
+
+ /** @var string */
+ private $name;
+
+ /** @var array */
+ private $attributes;
+
+ public function __construct(string $name, array $attributes = [])
+ {
+ $this->name = $name;
+ $this->attributes = $attributes;
+ }
+
+ public function name(): string
+ {
+ return $this->name;
+ }
+
+ public function attributes(): array
+ {
+ return $this->attributes;
+ }
+
+ public function toAvro(): array
+ {
+ $avro[AvroSchema::LOGICAL_TYPE_ATTR] = $this->name;
+ return array_merge($avro, $this->attributes);
+ }
+
+ public static function decimal(int $precision, int $scale): self
Review Comment:
You may want to add a validation for positive precision and scale here
##########
lang/php/lib/Schema/AvroSchema.php:
##########
@@ -304,7 +342,40 @@ public static function realParse($avro, $default_namespace
= null, &$schemata =
$type = $avro[self::TYPE_ATTR] ?? null;
if (self::isPrimitiveType($type)) {
- return new AvroPrimitiveSchema($type);
+ switch ($avro[self::LOGICAL_TYPE_ATTR] ?? null) {
+ case self::DECIMAL_LOGICAL_TYPE:
+ $precision =
$avro[AvroLogicalType::ATTRIBUTE_DECIMAL_PRECISION] ?? null;
+ if (!is_numeric($precision)) {
+ throw new AvroSchemaParseException(
+ 'Decimal logical type requires valid
"precision" attribute.'
+ );
+ }
+ $scale =
$avro[AvroLogicalType::ATTRIBUTE_DECIMAL_SCALE] ?? null;
+ if (!is_numeric($scale)) {
+ throw new AvroSchemaParseException(
+ 'Decimal logical type requires valid "scale"
attribute.'
+ );
+ }
Review Comment:
Please also check the following:
* precision > 0
* scale >= 0
* scale <= precision
* For bytes decimal: precision fits within reasonable bounds
* For fixed decimal: precision <= floor(log10(2^(8*size) - 1))
##########
lang/php/lib/Datum/AvroIOBinaryEncoder.php:
##########
@@ -187,4 +188,46 @@ public function writeBytes($bytes)
$this->writeLong(strlen($bytes));
$this->write($bytes);
}
+
+ public function writeDecimal($decimal, int $scale, int $precision): void
+ {
+ if (!is_numeric($decimal)) {
+ throw new AvroException('Decimal must be a numeric value');
+ }
+
+ $value = $decimal * (10 ** $scale);
+ if (!is_int($value)) {
+ $value = (int) round($value);
+ }
+ if (abs($value) > (10 ** $precision - 1)) {
+ throw new AvroException('Decimal value is out of range');
+ }
+
+ $packed = pack('J', $value);
Review Comment:
pack('J', ...) is machine-endian and width-dependent
Avro decimal bytes must be big-endian two’s complement
##########
lang/php/lib/Schema/AvroFixedSchema.php:
##########
@@ -67,4 +67,17 @@ public function toAvro()
$avro[AvroSchema::SIZE_ATTR] = $this->size;
return $avro;
}
+
+ public static function duration(
+ AvroName $name,
+ ?string $doc,
+ AvroNamedSchemata &$schemata = null,
+ ?array $aliases = null
+ ): self {
+ $fixedSchema = new self($name, $doc, 12, $schemata, $aliases);
+
+ $fixedSchema->logicalType = AvroLogicalType::duration();
Review Comment:
Aren't dynamic properties deprecated in PHP 8.2 ?
##########
lang/php/lib/Datum/AvroIODatumWriter.php:
##########
@@ -76,7 +77,7 @@ public function writeData($writers_schema, $datum, $encoder)
* @param AvroSchema $writers_schema
* @param $datum
* @param AvroIOBinaryEncoder $encoder
- * @return mixed
+ * @return void
Review Comment:
Shall we remove all `return`s below and maybe add `: void` as return type ?
##########
lang/php/lib/Schema/AvroSchema.php:
##########
@@ -64,12 +64,12 @@ class AvroSchema
public const INT_MAX_VALUE = 2147483647;
/**
- * @var long lower bound of long values: -(1 << 63)
+ * @var int lower bound of long values: -(1 << 63)
*/
public const LONG_MIN_VALUE = -9223372036854775808;
/**
- * @var long upper bound of long values: (1 << 63) - 1
+ * @var int upper bound of long values: (1 << 63) - 1
Review Comment:
Why `int`?
##########
lang/php/lib/Datum/AvroIOBinaryEncoder.php:
##########
@@ -187,4 +188,46 @@ public function writeBytes($bytes)
$this->writeLong(strlen($bytes));
$this->write($bytes);
}
+
+ public function writeDecimal($decimal, int $scale, int $precision): void
+ {
+ if (!is_numeric($decimal)) {
+ throw new AvroException('Decimal must be a numeric value');
+ }
+
+ $value = $decimal * (10 ** $scale);
+ if (!is_int($value)) {
+ $value = (int) round($value);
+ }
+ if (abs($value) > (10 ** $precision - 1)) {
Review Comment:
This looks wrong.
`precision` is the total number of digits.
Example: `9.9` has precision=2 and scale=1
Let's say we have a logicalType=decimal with precision=2 and scale=1.
`abs(98.9) > 10 ** 2 - 1` => `abs(98.9) > 99` would pass but `98.9` has
precision=3.
Possible improvement:
```php
$maxValue = 10 ** $precision;
if (abs($value) >= $maxValue) {
throw new AvroException('Decimal value is out of range');
}
```
##########
lang/php/lib/Schema/AvroSchema.php:
##########
@@ -64,12 +64,12 @@ class AvroSchema
public const INT_MAX_VALUE = 2147483647;
/**
- * @var long lower bound of long values: -(1 << 63)
+ * @var int lower bound of long values: -(1 << 63)
Review Comment:
Why `int` ?
--
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]
