Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Review org.opengis.filter package in light of OGC 09-026 #32

Closed
desruisseaux opened this issue May 5, 2018 · 7 comments
Closed

Review org.opengis.filter package in light of OGC 09-026 #32

desruisseaux opened this issue May 5, 2018 · 7 comments
Assignees

Comments

@desruisseaux
Copy link
Contributor

desruisseaux commented May 5, 2018

Review the org.opengis.filter package for conformance with OGC 09-026 specification. Additional changes to consider:

  • Remove the visitor pattern, to be considered as an implementation details.
  • Parametrized the Expression type with input and output types of evaluate method.
  • Consider removing evaluate(Object, Class) after above-cited parametrization.
  • Leverage java.util.function package (new since Java 8).
  • Merge FilterFactory2 into FilterFactory (after simplification).
  • Replace minx, maxx, miny, maxy arguments by Envelope.
  • Replace String srs arguments by CoordinateReferenceSystem.
  • Replace String units arguments by Unit.
  • Replace double distance, String units arguments by Length.

Note that the filter package is in the "pending" part of GeoAPI and was not part of GeoAPI 3.0 standard. This leave us more freedom to modify it.

@desruisseaux
Copy link
Contributor Author

Some experiments show that the way OGC filter and expression interfaces are currently defined in our Java profile leads to unsafe casts in implementations. We should consider being more specific on the expected input and output types. Example for a PropertyName expression expecting a Feature in input and returning a value or arbitrary type in output:

interface PropertyName<R> extends Expression<Feature, R>

@desruisseaux
Copy link
Contributor Author

Another change to consider: keep base types like BinaryExpression but delete the various subtypes (Multiply, Subtract, etc); instead we could use a code list. I'm not aware of situations were type safety is required, i.e. there is no API requiring at compile-time an expression to be a multiplication. Quite the opposite, those types block optimizations in FilterFactory. For example the fact that multiply(Expression, Expression) returns a Multiply object block us from simplifying multiplication by 1, or multiplications that cancel a previous multiplications.

Reducing the number of types would also simplify the API and make easier for implementors to use java.util.function in the way they want (less classes for them to extend).

@desruisseaux
Copy link
Contributor Author

We should also delete org.opengis.filter.identity package. It seems to come from XML, while GeoAPI should be only about conceptual models. I do not see a justification for those FeatureId, GmlObjectId, ObjectId or RecordId interfaces all duplicating the same functionality with no relevant additional information.

asfgit pushed a commit to apache/sis that referenced this issue Jun 8, 2019
…mentations.

Also try to perform a more elaborated analysis of types during arithmetic operations.
This work is not final; we still need to do a deeper review of filter interfaces:
opengeospatial/geoapi#32
@alexismanin
Copy link

It could be useful to reword some vocable. For example, property and geometric equality operators are very close to each other, and that can be mileading.

To get a property equality operator, one must call equal function. for geometric equality, one must call equals .
Moreover, return type for geometric equality is not more helpful, as it returns an Equal object.

Maybe an ST (Spatio-temporal, inspired by SQLMM) prefix would help see the difference.

@desruisseaux desruisseaux self-assigned this Mar 3, 2021
@desruisseaux
Copy link
Contributor Author

A proposal is taking shape in the refactor/filters branch. It happens that the UML in ISO 19143 is quite different than the XSD, and the model specified by the UML is very close to above suggestions. In particular the UML does not declare sub-types like Multiply, Add, etc. so removing them as suggested above actually makes GeoAPI more compliant with abstract model in OGC/ISO standards. OGC/ISO models use code lists instead than those sub-types.

Using those code lists also resolve the problem mentioned by @alexismanin. There is no longer confusion between the various equal functions because they are defined in different code lists.

desruisseaux added a commit that referenced this issue Apr 14, 2021
- Remove the visitor pattern.
- Remove non-standard subtypes (`IncludeFilter`, `ExcludeFilter`, `NilExpression`, etc.)
- Retrofit `Filter` as a subtype of `java.util.function.Predicate`.
- Retrofit `Expression` as a subtype of `java.util.function.Function`.
- Add exceptions defined in ISO 19143.

#32
@desruisseaux
Copy link
Contributor Author

desruisseaux commented Apr 14, 2021

During the upgrade to ISO 19143, we noticed the following issues with the OGC/ISO specification. But I did not found a working group for OGC Filter encoding where to report those issues.

ISO 19143 / OGC 09-028 issues

PropertyIsLike operator (§7.7.3.4)

The UML and the XSD said that this operator has 2 expressions, but does not said which one is the pattern. Common practice seems to use the second expression as the pattern.

Temporal operators (§7.9)

The TemporalOperatorName code list in the UML (figure 7) and the table 4 do not have the Ends element. But that elements is present in the XSD, as expected for consistency with Begins. I presume that it has been forgotten in the UML and in the table.

Logical operators (§7.10)

The operands property in BinaryLogicOperator seems to be restricted to 2 elements in the UML (figure 8), while it is unbounded in the XSD (2 is declared as only the minimum of elements).

Object identifiers (§7.11)

  • Typo in the UML (figure 9): RecourceId should be ResourceId (the XSD use the correct spelling).
  • The XSD and the discussion text have a previousRid attribute which does not appear in the UML.

Filter capabilities (§7.13)

ImplementsResourceld is missing in UML of Conformance (figure 11). That element appears in table 5.

Temporal capabilities (§7.14.6)

In the following sentence (emphasis is mine):

Temporal capabilities include the ability to filter temporal data of specified temporal types based on the spatial operators declared in this International Standard: After, Before, Begins, BegunBy, TContains, During, TEquals, TOverlaps, Meets, OverlappedBy, MetBy and EndedBy.

"Spatial" should be "temporal".

In the above-cited list of operators and in the UML at figure 15, "Ends" and "AnyInteracts" are missing.

Functions (§7.14.7)

The XSD defines 2 types: AvailableFunctionsType and AvailableFunctionType (plural and singular form respectively). But the UML defines only one type: AvailableFunctions (plural) with properties that correspond to AvailableFunctionType (singular) in the XSD. I propose the following changes:

  • In the UML at figure 16, rename AvailableFunctions as AvailableFunction.
  • In the UML at figure 10, change the type of functions property from AvailableFunctions to AvailableFunction and change the cardinality from [0…1] to [0…*].

Sorting (§8)

The UML is missing in OGC specification, but is present in ISO specification. I guess it is due to a broken link in AsciiDoc.

Annex E. Abstract model

Section E.2 should probably have a TemporalPredicate case for completness.

Feature API draft standard

In OGC API - Features - Part 3: Filtering and the Common Query Language (CQL), requirement 18G said:

If the SINGLECHAR modifier is not specified, the default singlechar character SHALL be "_".

But in Annex C: JSON schemas for CQL (Normative) subsection C1 and C2, the schema declares a dot instead of underscore character:

"singleChar": { "type": "string", "default": "." }

(Edit April 30th): Above issue about the _ character appears to be already reported in opengeospatial/ogcapi-features#553.

desruisseaux added a commit that referenced this issue Apr 18, 2021
- Remove the visitor pattern.
- Remove non-standard subtypes (`IncludeFilter`, `ExcludeFilter`, `NilExpression`, etc.)
- Retrofit `Filter` as a subtype of `java.util.function.Predicate`.
- Retrofit `Expression` as a subtype of `java.util.function.Function`.
- Add exceptions defined in ISO 19143.

#32
desruisseaux added a commit that referenced this issue Jun 22, 2021
- Provide a way to specify the expected type of value computed by expressions.
- Add explicit method in `BinaryComparisonOperator` for getting operands.
- Revision of `FilterFactory` interface. It stay in "pending" section because parameterized types may need to be revisited.

#32
desruisseaux added a commit that referenced this issue Jun 22, 2021
Add an entry for "covariant type" in definition of terms.
Add a note below UML saying that it shows type covariance.

#32
asfgit pushed a commit to apache/sis that referenced this issue Jun 22, 2021
- `FilterVisitor` and `ExpressionVisitor` removed
- `IncludeFilter` and `ExcludeFilter` removed from public API
- `NilExpression` removed
- `Filter` as a sub-type of `java.util.Predicate`
- `Expression` as a sub-type of `java.util.Function`

Implementation changes;
- Moved JTSMapping method to a `Wrapper.toGeometryType(…)` method.
- Remove some classes and methods no longer used.
- Refactor SQLMM test in a more implementation neutral (JTS versus ESRI) way.
- Initial `Optimization` support.

opengeospatial/geoapi#32
@desruisseaux
Copy link
Contributor Author

Branch refactor/filters merged to master.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants