Skip to content
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

Range and MultiRange parser #307

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Gerych1984
Copy link

Q A
Is bugfix?
New feature? ✔️
Breaks BC?

Range and MultiRange format parser for PgSql

@what-the-diff
Copy link

what-the-diff bot commented Aug 2, 2023

PR Summary

  • Enhancements to ColumnSchema.php
    • Added a couple of new methods to this file called getRangeParser() and getMultiRangeParser(). These methods provide objects that help in interpreting range and multi-range values from the database.
  • Introduction of MultiRangeParser.php & RangeParser.php
    • These new files introduce classes that assist in parsing and understanding values representing a series of values (ranges) and a set of such series (multi-ranges) specifically in PostgreSQL databases.
  • Updates to Schema.php
    • Some constants specific to understanding range column types have been added. These definitions improve the functionality of the newly introduced RangeParser & MultiRangeParser classes.
  • Addition of MultiRangeParserTest.php & RangeParserTest.php
    • These new files consist of test scenarios for the new classes MultiRangeParser and RangeParser respectively. They ensure the logic driving these classes' operation works as expected.

@codecov
Copy link

codecov bot commented Aug 2, 2023

Codecov Report

Patch coverage: 94.35% and project coverage change: -0.97% ⚠️

Comparison is base (adcb2ee) 100.00% compared to head (5b96d65) 99.03%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@              Coverage Diff              @@
##              master     #307      +/-   ##
=============================================
- Coverage     100.00%   99.03%   -0.97%     
- Complexity       200      272      +72     
=============================================
  Files             13       15       +2     
  Lines            596      722     +126     
=============================================
+ Hits             596      715     +119     
- Misses             0        7       +7     
Files Changed Coverage Δ
src/Schema.php 100.00% <ø> (ø)
src/RangeParser.php 92.50% <92.50%> (ø)
src/MultiRangeParser.php 96.87% <96.87%> (ø)
src/ColumnSchema.php 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Tigrov
Copy link
Member

Tigrov commented Aug 2, 2023

Good idea to support range types 👍

  1. What about user defined range types?
    https://www.postgresql.org/docs/current/rangetypes.html
    8.17.8. Defining New Range Types
  2. You should get what you insert. Do not increment or decrement the value. It is user's responsibility to have inclusive or exclusive values.
  3. There is PHP class DatePeriod for date range. What about to use it for convertation?

@Gerych1984
Copy link
Author

Gerych1984 commented Aug 2, 2023

Good idea to support range types 👍

  1. What about user defined range types?
    https://www.postgresql.org/docs/current/rangetypes.html
    8.17.8. Defining New Range Types
  2. You should get what you insert. Do not increment or decrement the value. It is user's responsibility to have inclusive or exclusive values.
  3. There is PHP class DatePeriod for date range. What about to use it for convertation?
  1. We need to think about this. The main problem is how to understand when it is necessary to run the data by this parser in a custom format
  2. Need increment/decrement - from personal experience. If postgres returned a string (2,8), it is actually a range from 3 to 7 (and the user entered it)
  3. good point, need to read/think about it. The array was left over from the first iteration where 7.4 was supported. There are a lot more possibilities now

@Gerych1984
Copy link
Author

  1. There is PHP class DatePeriod for date range. What about to use it for convertation?

Unfortunately DatePeriod can't be without StartDate, but *range can

@Tigrov
Copy link
Member

Tigrov commented Aug 3, 2023

  1. We need to think about this. The main problem is how to understand when it is necessary to run the data by this parser in a custom format
  2. Need increment/decrement - from personal experience. If postgres returned a string (2,8), it is actually a range from 3 to 7 (and the user entered it)
  3. good point, need to read/think about it. The array was left over from the first iteration where 7.4 was supported. There are a lot more possibilities now
  1. In Schema class
...
public const TYPE_RANGE = 'range';
...
protected function loadColumnSchema(array $info): ColumnSchemaInterface
{
...
    if ($info['type_type'] === 'r') {
        $column->type(self::TYPE_RANGE);
    }
...
}
...

Then you can check $column->getType() === Schema::TYPE_RANGE
2. Ok, looks increment/decrement is required
3. Got it

@Gerych1984
Copy link
Author

  1. In Schema class
...
public const TYPE_RANGE = 'range';
...
protected function loadColumnSchema(array $info): ColumnSchemaInterface
{
...
    if ($info['type_type'] === 'r') {
        $column->type(self::TYPE_RANGE);
    }
...
}
...

Then you can check $column->getType() === Schema::TYPE_RANGE

A while back I suggested adding a custom type mapper. For example, it would be more convenient for me if geometric types were immediately given as an instance of a suitable class from a given library. I think it would be more universal than to have to make a whole package for such a special case

For example somewhere in di:

$schema->setType('point', static fn (?string $value) => $value ? Point::fromText($value) : null );

@Gerych1984 Gerych1984 force-pushed the range-parser branch 7 times, most recently from 705193b to 39ec0a4 Compare August 7, 2023 18:43
Copy link
Member

@Tigrov Tigrov left a comment

Choose a reason for hiding this comment

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

  1. In Schema class
...
public const TYPE_RANGE = 'range';
...
protected function loadColumnSchema(array $info): ColumnSchemaInterface
{
...
    if ($info['type_type'] === 'r') {
        $column->type(self::TYPE_RANGE);
    }
...
}
...

Then you can check $column->getType() === Schema::TYPE_RANGE

Suggestion to realize this way.

$schema->setType('point', static fn (?string $value) => $value ? Point::fromText($value) : null );

Something like this will be possible after this PR yiisoft/db#752 in version 2.0.0

This will allow to create PointColumn class and support type conversation there.

Comment on lines +92 to +103
public const TYPE_INT_RANGE = 'int4range';
public const TYPE_BIGINT_RANGE = 'int8range';
public const TYPE_NUM_RANGE = 'numrange';
public const TYPE_TS_RANGE = 'tsrange';
public const TYPE_TS_TZ_RANGE = 'tstzrange';
public const TYPE_DATE_RANGE = 'daterange';
public const TYPE_INT_MULTIRANGE = 'int4multirange';
public const TYPE_BIGINT_MULTIRANGE = 'int8multirange';
public const TYPE_NUM_MULTIRANGE = 'nummultirange';
public const TYPE_TS_MULTIRANGE = 'tsmultirange';
public const TYPE_TS_TZ_MULTIRANGE = 'tstzmultirange';
public const TYPE_DATE_MULTIRANGE = 'datemultirange';
Copy link
Member

@Tigrov Tigrov Nov 9, 2023

Choose a reason for hiding this comment

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

Preffered simple abstract types instead of list of all supported range db types.

Suggestion to use one or two abstract types

Suggested change
public const TYPE_INT_RANGE = 'int4range';
public const TYPE_BIGINT_RANGE = 'int8range';
public const TYPE_NUM_RANGE = 'numrange';
public const TYPE_TS_RANGE = 'tsrange';
public const TYPE_TS_TZ_RANGE = 'tstzrange';
public const TYPE_DATE_RANGE = 'daterange';
public const TYPE_INT_MULTIRANGE = 'int4multirange';
public const TYPE_BIGINT_MULTIRANGE = 'int8multirange';
public const TYPE_NUM_MULTIRANGE = 'nummultirange';
public const TYPE_TS_MULTIRANGE = 'tsmultirange';
public const TYPE_TS_TZ_MULTIRANGE = 'tstzmultirange';
public const TYPE_DATE_MULTIRANGE = 'datemultirange';
public const TYPE_RANGE = 'range';
public const TYPE_MULTIRANGE = 'multirange';

Comment on lines +14 to +73
private const RANGES = [
Schema::TYPE_INT_RANGE,
Schema::TYPE_BIGINT_RANGE,
Schema::TYPE_NUM_RANGE,
Schema::TYPE_TS_RANGE,
Schema::TYPE_TS_TZ_RANGE,
Schema::TYPE_DATE_RANGE,
];

private ?string $type;

public function __construct(?string $type = null)
{
$this->type = $type;
}

public function withType(?string $type): self
{
$new = clone $this;
$new->type = $type;

return $new;
}

public function asInt(): self
{
return $this->withType(Schema::TYPE_INT_RANGE);
}

public function asBigInt(): self
{
return $this->withType(Schema::TYPE_BIGINT_RANGE);
}

public function asNumeric(): self
{
return $this->withType(Schema::TYPE_NUM_RANGE);
}

public function asDate(): self
{
return $this->withType(Schema::TYPE_DATE_RANGE);
}

public function asTimestamp(): self
{
return $this->withType(Schema::TYPE_TS_RANGE);
}

public function asTimestampTz(): self
{
return $this->withType(Schema::TYPE_TS_TZ_RANGE);
}

public function asCustom(): self
{
return $this->withType(null);
}

public function parse(?string $value): ?array
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private const RANGES = [
Schema::TYPE_INT_RANGE,
Schema::TYPE_BIGINT_RANGE,
Schema::TYPE_NUM_RANGE,
Schema::TYPE_TS_RANGE,
Schema::TYPE_TS_TZ_RANGE,
Schema::TYPE_DATE_RANGE,
];
private ?string $type;
public function __construct(?string $type = null)
{
$this->type = $type;
}
public function withType(?string $type): self
{
$new = clone $this;
$new->type = $type;
return $new;
}
public function asInt(): self
{
return $this->withType(Schema::TYPE_INT_RANGE);
}
public function asBigInt(): self
{
return $this->withType(Schema::TYPE_BIGINT_RANGE);
}
public function asNumeric(): self
{
return $this->withType(Schema::TYPE_NUM_RANGE);
}
public function asDate(): self
{
return $this->withType(Schema::TYPE_DATE_RANGE);
}
public function asTimestamp(): self
{
return $this->withType(Schema::TYPE_TS_RANGE);
}
public function asTimestampTz(): self
{
return $this->withType(Schema::TYPE_TS_TZ_RANGE);
}
public function asCustom(): self
{
return $this->withType(null);
}
public function parse(?string $value): ?array
public function parse(?string $value, ?string $dbType = null): ?array

Suggestion to pass $dbType to parse() method instead of initiating

Comment on lines +151 to +152
$min = $lower ? DateTime::createFromFormat('Y-m-d', $lower) : null;
$max = $upper ? DateTime::createFromFormat('Y-m-d', $upper) : null;
Copy link
Member

Choose a reason for hiding this comment

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

Need to decide this will be DateTime or better DateTimeImmutable
or just string due to yiisoft/db do not support DateTime object.
And DateTimeImmutable can be added after solving the issue yiisoft/db#725 perhaps in version 2.0.0

Comment on lines +107 to +113
if (is_string($value) && $rangeParser = $this->getRangeParser()) {
return $rangeParser->parse($value);
}

if (is_string($value) && $multiRangeParser = $this->getMultiRangeParser()) {
return $multiRangeParser->parse($value);
}
Copy link
Member

@Tigrov Tigrov Nov 9, 2023

Choose a reason for hiding this comment

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

Suggested change
if (is_string($value) && $rangeParser = $this->getRangeParser()) {
return $rangeParser->parse($value);
}
if (is_string($value) && $multiRangeParser = $this->getMultiRangeParser()) {
return $multiRangeParser->parse($value);
}

Please move this to phpTypecastValue() method in section return match ($this->getType()) {...}

Copy link
Member

@Tigrov Tigrov left a comment

Choose a reason for hiding this comment

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

Also need to implement dbTypecast(mixed $value) to cast PHP array of range value to DB. It is better to implement an ExpressionInterface e.g. RangeExpression and MultirangeExpression

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.

2 participants