Take the field index, not name, in the feature getters and setters

[lnicola]

• I agree to follow the project's code of conduct.
• I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

---

Field access by name tends to be very slow (I've seen it take 30% of the time in some programs), but beginners are unlikely to realize that they're doing a number of expensive lookups per feature.

Summary of changes:

• drop field getters that take a field name
• change the field setters to take an index instead of a name (the index versions were missing)
• make field_count return usize for consistency
• add Defn::field_index and make Feature::field_index public
• drop LayerAccess::create_feature_fields (it could be updated, but it's a bit weird)

[lnicola]

r? @metasim, @ChristianBeilschmidt, @jdroenner since this is a relatively big change
r? @latot since you said you wanted to get involved

Hopefully I'll be able to get rid of this thing soon:

(it goes on and on)

[latot]

I think this is nice, some questions... can we actually change the fields in any way that could broke the field id?

From where is the image? I can see some transformations from isize to i32 with as.

[lnicola]

can we actually change the fields in any way that could broke the field id?

You can delete fields from the Defn of a layer, but it won't reflect in the fields of the features, and I don't think we even expose that.

From where is the image? I can see some transformations from isize to i32 with as.

From usize, maybe, but most of those are checked (I think layer iteration is the exception). GDAL fields are 32-bit and positive (-1 means a missing field), but Rust uses probably expect usize. We do a similar thing for raster dimensions. But it's a small change and can be reverted easily (either now or in the future).

Or we could go the other way around and introduce a FieldIdx type that wraps an i32.

[lnicola]

Gentle nudge @jdroenner 😅.

[latot]

I thinking the next thing.

Actually you said there is no function exposed that would be able to change the columns (remove/add), at some point I think would be good to have some tests that checks the actual idx do not change in any of this operations, just to be sure this is going to work fine in the future too.

No idea if rn is the best moment.

[lnicola]

@latot if we don't expose OGR_L_DeleteField, how could we test that it doesn't affect the other indexes? Note that the GDAL docs explicitly say:

This function should not be called while there are feature objects in existence that were obtained or created with the previous layer definition.

Keep in mind that the intended usage is something like:

let id_idx = layer.defn().field_index("id")?;
let name_idx = layer.defn().field_index("name")?;
// you could call `layer.defn().delete_field(name_idx)?` here if it existed, but why?

for feature in layer.features() {
    let id = feature.field_as_integer(id_idx)?.expect("id is set");
    let name = feature.field_as_string(id_idx)?.expect("name is set");
    println!("Feature {id}, name: {name}");

    // you could never call `layer.defn().delete_field(name_idx)?` here because Rust
}

[latot]

Yes, the would be the usage, is more like a note to when something that could affect the id is exposed to the user, also, the only side I'm seeing, some ppl, could try to call that function directly even if is not exposed, would be the last concern from me.

[lnicola]

We can't prevent every bug. You can always pass in the wrong field index or name, or even delete the whole layer. But if you do, you'll notice pretty quickly once you run your program.

What's harder to notice is that your program is running twice as slow because you've used the wrong field access methods.