Remote widget calculation for spatial indexed sources #898

zbigg · 2024-07-31T09:33:35Z

Description

Shortcut: https://app.shortcut.com/cartoteam/story/425306

Spatial index sources use remote widgets calculation.
addSource: Support for spatialDataType, spatialIndexColumn instead of old geoColumn, at least for remote widgets
remote widgets: support for new parameters spatialDataType, spatialIndexColumn, spatialFiltersResolution and spatialFiltersMode

Type of change

Feature

Acceptance

...

Basic checklist

Good PR name
Shortcut link
Changelog entry
Just one issue per PR
GitHub labels
Proper status & reviewers
Tests
Documentation

…spatialFiltersResolution

…calculation

…olumn, spatialFiltersResolution

github-actions · 2024-07-31T10:00:50Z

Pull Request Test Coverage Report for Build 10722661073

Details

21 of 38 (55.26%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.2%) to 70.957%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/react-api/src/api/model.js	16	22	72.73%
packages/react-widgets/src/hooks/useWidgetFetch.js	5	16	31.25%

Totals
Change from base Build 10145530283:	-0.2%
Covered Lines:	2835
Relevant Lines:	3678

💛 - Coveralls

github-actions · 2024-07-31T10:01:52Z

Visit the preview URL for this PR (updated for commit c5e9740):

https://cartodb-fb-storybook-react-dev--pr898-feature-sc-42530-68u0achy.web.app

_{(expires Thu, 12 Sep 2024 14:47:06 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: 517cc4d31d7e09cf277774e034094b67c301cd4c}

jmgaya

Everything seems correct and the enhancements seem correct for supporting spatial indexes, at least from my side/knowledge, great job!

jmgaya · 2024-09-03T14:00:53Z

packages/react-api/src/api/model.js

    queryParams.spatialFilters = JSON.stringify(spatialFilters);
+    queryParams.spatialDataType = spatialDataType;
+    if (spatialDataType !== 'geo') {


I see geo repeated a few times in this file, so what do you think about extracting it somewhere and naming it as something like const DEFAULT_SPATIAL_DATA_TYPE?

jmgaya · 2024-09-03T14:01:45Z

packages/react-api/src/api/model.js

    queryParams.spatialFilters = JSON.stringify(spatialFilters);
+    queryParams.spatialDataType = spatialDataType;
+    if (spatialDataType !== 'geo') {
+      if (source.spatialFiltersResolution !== undefined) {


Out of curiosity: Could source.spatialFiltersResolution be null at some point? If so, maybe we can double negate this expression to prevent this case, or is there any edge case related to a 0 resolution?

Well, this we can't protect user from themselves.

If he passed number, he's ok. If he didn't passed anything (undefined), he's ok - default is used. If he passed null or object or date, it's his own fault - i don't believe that we should check every possible failure mode if we already defined that spatialFiltersResolution is optional number.

This is just API wrapper, nothing will explode - just weird stuff will be sent to.

(and btw, this is internal code - validation if required should be placed somewhere on top of widget)

I was thinking more about something like !!source.spatialFiltersResolution, WDYT?

I don't like !!source.spatialFiltersResolution for one reason: who said, 0 is bad value for this parameter (is it?). I only care if user provided it or not - and that's why we have undefined in JS.

packages/react-api/src/types.d.ts

jmgaya · 2024-09-03T14:05:22Z

packages/react-widgets/src/hooks/useWidgetFetch.js

@@ -93,6 +107,36 @@ export default function useWidgetFetch(
    [global, viewport, spatialFilter]
  );

+  const source2 = useMemo(() => {


Probably a more descriptive name for this parameter would help our future ourselves? What about enrichedSource?

donmccurdy · 2024-09-03T17:33:12Z

packages/react-api/src/api/model.js

+      if (source.geoColumn) {
+        const parsedGeoColumn = source.geoColumn ? source.geoColumn.split(':') : [];


Checking source.geoColumn redundantly here – probably one or the other isn't required?

donmccurdy · 2024-09-03T17:33:52Z

packages/react-api/src/api/model.js

+        } else if (parsedGeoColumn.length === 1) {
+          spatialDataColumn = parsedGeoColumn[0] || DEFAULT_GEO_COLUMN;
+          spatialDataType = 'geo';
+        }


Could parsedGeoColumn really be []? If so do we need a third case for it?

I don't think so (and i've checked as i wasn't sure):

> "".split(':') [''] > "a".split(':') ['a'] > "a:b".split(':') ['a', 'b']

donmccurdy · 2024-09-03T17:36:45Z

packages/react-api/src/types.d.ts

+  dataResoultion?: number;
+  spatialDataType?: string;
+  spatialDataColumn?: string;
+  spatialFiltersResolution?: number;


The spatialFilter parameter is singular, not plural – assuming these are related, should this be spatialFilterResolution instead? Note that this may become a public-facing API once added to @carto/api-client, though I do see the note in the AD that we may not expose it.

spatialFiltersResolution is term already used in platform maps API. Didn't invent this name

I think, creating another slightly different symbol would be confusing, so i would leave it as it is.

~~@thedae @josemaria-vilaplana I leave this comment to You. Maybe we should broaden reviews of new platform API that is about to be visible in many places us for long time~~
(wrong, see below)

Sorry, platform APIs already spell this as spatialFilters so naming inconsitency is only visible in "carto4react" source API.

@donmccurdy I think we're free to fix it in new frontend packages/APIs. I would leave it as it is in c4r which - i believe - is in deprecated mode and we don't refactor things here anymore

Ok — sounds good to me!

donmccurdy · 2024-09-03T17:38:47Z

packages/react-widgets/src/hooks/useWidgetFetch.js

+// stolen from deck.gl/modules/carto/src/layers/h3-tileset-2d.ts
+const BIAS = 2;
+export function getHexagonResolution(viewport, tileSize) {
+  // Difference in given tile size compared to deck's internal 512px tile size,
+  // expressed as an offset to the viewport zoom.
+  const zoomOffset = Math.log2(tileSize / 512);
+  const hexagonScaleFactor = (2 / 3) * (viewport.zoom - zoomOffset);
+  const latitudeScaleFactor = Math.log(1 / Math.cos((Math.PI * viewport.latitude) / 180));
+
+  // Clip and bias
+  return Math.max(0, Math.floor(hexagonScaleFactor + latitudeScaleFactor - BIAS));
+}
+


(no action required) @felixpalmer maybe this is something we'll want to pull into @carto/api-client to reuse in both places? Once the repository is public and can be made a dependency, that is.

Hmm, wouldn't it be better to export it from deck? It's a key part of how H3Tileset works

Oh, yes – that does sound better. :)

donmccurdy · 2024-09-03T17:43:56Z

packages/react-redux/src/slices/cartoSlice.js

@@ -204,6 +204,8 @@ export const createCartoSlice = (initialState) => {
 * @param {FiltersLogicalOperators=} data.filtersLogicalOperator - logical operator that defines how filters for different columns are joined together.
 * @param {import('@deck.gl/carto').QueryParameters} data.queryParameters - SQL query parameters.
 * @param {string=} data.geoColumn - (optional) name of column containing geometries or spatial index data.
+ * @param {number=} data.dataResolution - data resolution for spatial index data.


Is this connected to spatialDataType and spatialDataColumn, and would spatialDataResolution be a clearer name if so? For my own learning, is more information on what the parameters do available somewhere?

We don't understand connection "well" enough yet.

dataResolution is property of tileset - namely, maximum resolution of h3 or quadbin index encountered in table.

To apply filters, we need to apply some resolution that is not bigger than dataResolution

The idea is that we allow users to provide two:

dataResolution - so c4r is smart enough to determine some resolution from property received in map instantiation (dataResolution exists in tilejson response)

spatialFiltersResolution - if user (like builder) wants to control this value directly.

That's why we have this getHexagonResolution - it's only used when spatialFiltersResolution is needed (h3/quadbin) and user didn't provided it, so we derive it from dataResolution.

Ok, thanks! I guess it's a request for the API team more, but a glossary in docs would be really helpful. 😓

jmgaya · 2024-09-05T13:26:18Z

packages/react-api/src/api/model.js

    queryParams.spatialFilters = JSON.stringify(spatialFilters);
+    queryParams.spatialDataType = spatialDataType;
+    if (spatialDataType !== 'geo') {
+      if (source.spatialFiltersResolution !== undefined) {


I was thinking more about something like !!source.spatialFiltersResolution, WDYT?

zbigg added 4 commits July 31, 2024 11:29

react-api(executeModel): support spatialDataType, spatialDataColumn, …

4eb8aab

…spatialFiltersResolution

react-widgets: dynamic spatial index based sources use remote widget …

1975cc3

…calculation

fixup! react-api(executeModel): support spatialDataType, spatialDataC…

f05b84c

…olumn, spatialFiltersResolution

changelog update

14cad2d

zbigg requested review from padawannn, jmgaya and donmccurdy September 3, 2024 13:15

zbigg added 2 commits September 3, 2024 15:21

useWidgetFetch: adapt spatialFiltersResolution to current zoom level

cb289a9

useWidgetFetch: protect agains empty source

cdeb4cf

zbigg force-pushed the feature/sc-425306/c4r-support-for-spatial-indexes-and-proper branch from 1049f70 to cdeb4cf Compare September 3, 2024 13:21

jmgaya approved these changes Sep 3, 2024

View reviewed changes

donmccurdy reviewed Sep 3, 2024

View reviewed changes

jmgaya approved these changes Sep 5, 2024

View reviewed changes

zbigg added 2 commits September 5, 2024 15:30

CR + floor quadbin spatialFiltersResolution

debeec3

fix tests

c5e9740

zbigg merged commit fcfed20 into master Sep 5, 2024
2 checks passed

zbigg mentioned this pull request Sep 19, 2024

isRemoteCalculationSupported: revert remote calculation for dynamic spatial index sources #905

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remote widget calculation for spatial indexed sources #898

Remote widget calculation for spatial indexed sources #898

zbigg commented Jul 31, 2024

github-actions bot commented Jul 31, 2024 •

edited

Loading

github-actions bot commented Jul 31, 2024 •

edited

Loading

jmgaya left a comment

jmgaya Sep 3, 2024

jmgaya Sep 3, 2024

zbigg Sep 4, 2024

jmgaya Sep 5, 2024

zbigg Sep 5, 2024

jmgaya Sep 3, 2024

donmccurdy Sep 3, 2024

donmccurdy Sep 3, 2024

zbigg Sep 4, 2024

donmccurdy Sep 3, 2024

zbigg Sep 4, 2024 •

edited

Loading

zbigg Sep 4, 2024 •

edited

Loading

donmccurdy Sep 4, 2024

donmccurdy Sep 3, 2024

felixpalmer Sep 4, 2024

donmccurdy Sep 4, 2024

donmccurdy Sep 3, 2024

zbigg Sep 4, 2024 •

edited

Loading

donmccurdy Sep 4, 2024

jmgaya Sep 5, 2024

		if (source.geoColumn) {
		const parsedGeoColumn = source.geoColumn ? source.geoColumn.split(':') : [];

Remote widget calculation for spatial indexed sources #898

Remote widget calculation for spatial indexed sources #898

Conversation

zbigg commented Jul 31, 2024

Description

Type of change

Acceptance

Basic checklist

github-actions bot commented Jul 31, 2024 • edited Loading

Pull Request Test Coverage Report for Build 10722661073

Details

💛 - Coveralls

github-actions bot commented Jul 31, 2024 • edited Loading

jmgaya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zbigg Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

zbigg Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zbigg Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 31, 2024 •

edited

Loading

github-actions bot commented Jul 31, 2024 •

edited

Loading

zbigg Sep 4, 2024 •

edited

Loading

zbigg Sep 4, 2024 •

edited

Loading

zbigg Sep 4, 2024 •

edited

Loading