[GEOT-7245] ReTypingFeatureIterator should't demand equality among AttributeDescriptors

[roarbra]

Making the ReTypingFeatrueIterator more usable by reducing the limitations on attributes.
Fixes a problem where AttributeType names are different. Therefor is the test somewhat small.

Checklist

• I have read the contribution guidelines.
• I have sent a Contribution Licence Agreement (not required for small changes, e.g., fixing typos in documentation).
• First PR targets the main branch (backports managed later; ignore for branch specific issues).
• Avoid Java 9+ split packages.
• All the build checks are green (see automated QA checks).

For core and extension modules:

• New unit tests have been added covering the changes.
• Documentation has been updated (if change is visible to end users).
• There is an issue in GeoTools Jira (except for changes not visible to end users).
• Commit message(s) must be in the form [GEOT-XYZW] Title of the Jira ticket.
• Bug fixes and small new features are presented as a single commit.
• The commit targets a single objective (if multiple focuses cannot be avoided, each one is in its own commit, and has a separate ticket describing it).

[aaime]

@jodygarnett this PR seems to be up to alley, could you have a look?

[jodygarnett]

I actually wonder why the request is being made; the point of retyping feature iterator is to test for equality so that the final output matches exactly what is requested....

[jodygarnett]

I would like to understand your requirement / motivation more clearly?

I provided a small request to capture the relationship you wish to use as a method so it can be documented and tested.

[jodygarnett]

This appears considerably more relaxed.

• Q: What is AttributeDescriptor equals checking that is causing a problem?

• Q: If you wish to establish a different relationship "covers" I would be more comfortable if you added it to the one of the utilities class (like FeatureTypes.java so it can have a clear javadoc and test case.

[roarbra]

Yes, it is more relaxed, but it doesn't alter the original usage. If those SimpleFeatureType's are equal. They can still use this class. My change just makes it more useful for other situations.

Q: What is AttributeDescriptor equals checking that is causing a problem?
The test I included describes this. There is an inconsistency within AttributeDescriptor, that makes AttributeType.getName() either having the name of the descriptor or the name of the type. Both being as much useful for a retyping of the values.

[jodygarnett]

So you have:

• schema original with attribute descriptor "bar" / of property type "bar" with java binding Integer
• schema test with attribute descriptor "bar" / of property type "Integer" with java binding Integer

So this PR is in effect seeking to ignore the property type; and just focus on if the descriptor name match?

I think what you are proposing would be new functionality; all the ReType methods are really about sub-setting an existing schema (so we can ignore properties that the user is not interested in as part of their query).

Aside: We do have the ability to transform features on the fly here: https://github.com/geotools/geotools/tree/main/modules/extension/transform/src/main/java/org/geotools/data/transform

[roarbra]

Yes, it was an easy path to overcome a problem I've seen during my usage of GeoTools. Initially I tried with this #4069, but as @aaime pointed out. That was a bit overwhelming.

I still think this was a good proposal, but accept that you don't want it.

[jodygarnett]

Actually - I want it ... but it is a significant change that I think we need to go through the community proposal process.

They way I am thinking it is that:

1. retype: handles the common case where Query has been used to shuffle the order of the parameters around; or only select some of the parameters; and expects the java bindings to match. This would in effect be a "fast path" ...

2. covers: (can we think of a better name) relaxes this constraint so it just is based on descriptor name; and expects the java bindings to be assignable from (or even use a converter).

Options on how to engage:

• We could make a query hint to allow "relax=true", or "covers=true", or "strict=false"
• We could try ReType relationship (as a fast path); and if it is not suitable fall back to Covers relationship...

It seems like a useful idea?

[roarbra]

I see your point when you talk about a "fast path", but looking at the current code I would argue that we're not having a "fast path" at the moment. Within the "next" function there is a usage of SimpleFeatureBuilder, that in turn will call Converters.convert for all attributes of all the features.
I believe a "fast path" would do more like setting value directly from the original attribute into the new attribute. In such a case it would make perfect sense to demand equality among the AttributeType's. At the moment, I don't see the reason to demand this.

[aaime]

@roarbra the way this PR is evolving is worrying me... it looks like you're trying to do something that GeoTools was not designed for. Can you give us a bit of context into what practical problem you're trying to address? So far we have seen proposed solutions, but I think it would help to understand the problem they originate from.

[roarbra]

@aaime I've tried to recapture my original problem, and the reason to include this PR, but without success.
During the work with this PR, I've learned that my usage of the class ReTypingFeatureCollection was wrong. The correct usage seems to be:

targetSchema = SimpleFeatureTypeBuilder.retype(schema, propertyNames);
return new ReTypingFeatureCollection(original, targetSchema);

It would've been easier to realize this, if there was a constructor taking propertyNames as argument.

Still I think that my PR makes this class more usable.

[jodygarnett]

I am glad you are not stuck; I like the idea of making an enum / config option so let me think about how to do this cleanly.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically closed because it has not had recent activity. Please re-submit with updates if you want to resume work on this topic. Thank you for your contributions.