Spatial index widgets

[juandjara]

related story: https://app.shortcut.com/cartoteam/story/454268/adapt-carto-api-client-to-be-able-to-work-with-dynamic-spatial-index

[donmccurdy]

Thanks, this is nicely structured! A few comments/questions, still working my way though the code review. :)

[donmccurdy]

Primarily H3QuerySourceOptions represents the public-facing option types of the h3QuerySource function. Since users would never pass the 'spatialDataType' prop here, I think we can remove it from the 'H3QuerySourceOptions' definition.

(And similar for other sources)

[juandjara]

Okay, if we do this, where do we define the spatialDataType prop then?
Because the spatialDataColumn prop is defined in QuerySourceOptions and TableSourceOptions

[juandjara]

Other thing I was considering was moving this prop definition to WidgetBaseSourceProps

[donmccurdy]

The only consequence if it isn't defined explicitly (as on main branch now) is that the following...

const data = await h3QuerySource({..., spatialDataType: 'quadbin'});

... is a runtime error instead of a compile-time error. A compile-time error would be better, I agree, but the user must be able to import and use H3QuerySourceOptions themselves and the required/optional fields should match what h3QuerySource accepts. So perhaps instead of removing the override, we can just change its (still optional) value as spatialDataType?: 'h3' rather than spatialDataType: 'h3'?

export type H3QuerySourceOptions = SourceOptions &
  QuerySourceOptions &
  AggregationOptions &
  FilterOptions & {
    spatialDataType?: 'h3';
  };

[juandjara]

That's perfect, I was working on a second proposal for this, but this one is easier to understand

[donmccurdy]

Minor — Since viewState.zoom is required input, I'd slightly prefer not giving it a fallback here.

[juandjara]

Ok we will remove this variable then

[donmccurdy]

I'm not sure yet if we should do this, so no need to make the change now, but ... would it work if instead of asking the user to provide spatialFiltersResolution, we computed it internally from the WidgetBaseSource's own props, and a (new) viewState prop that the user would pass into getCategories etc.? Or is this something the user needs to directly control?

I'm a bit worried about all of the default values used inside 'getSpatialFiltersResolution', and things like tileSize, because it would be easy for the user to forget something. It would be nice if all the default values were populated before the 'source' is passed into the getSpatialFiltersResolution function, and this would make implementation in the widget web components simpler.

[juandjara]

Yes, user should be able to specify a concrete spatialFiltersResolution I think. I'm also a bit worried about all the conditional logic inside of that function, but since is inherited legacy code, I decided not to question it very much. We can find a way to adapt it if you would like.

[donmccurdy]

/cc @zbigg @felixpalmer does this concern make sense, and do you have any immediate preferences?

If we're all not sure then maybe it's best to go ahead and try setting up 1-2 of the example widgets (https://github.com/CartoDB/carto-api-client/tree/main/examples/components/widgets) to test with spatial index sources.

[juandjara]

Okay I'll let you know when I have the examples ready

[donmccurdy]

Does similar logic exist in C4R now, and could you share a link to it for comparison?

I'm not sure about using tileSize without also referring to tileResolution, that seems a bit strange to me, but maybe this is just how it already works?

[juandjara]

Yes this is almost copied verbatim from C4R. See here https://github.com/CartoDB/carto-react/blob/master/packages/react-widgets/src/models/spatialFiltersResolution.js#L43

[donmccurdy]

I see, thanks! I think at least for the aggregationResLevel we should make sure the user doesn't need to specify that twice (once for the source, and again for getSpatialFiltersResolution). But there's more than one way to do this, so I think it will depend on the question above.