Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Remote widget calculation for spatial indexed sources #898

Merged
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

## Not released

- Spatial Index Sources use remote widgets calculation [#898](https://github.com/CartoDB/carto-react/pull/898)

## 3.0.0

### 3.0.0-alpha.17 (2024-07-29)
Expand Down
43 changes: 37 additions & 6 deletions packages/react-api/src/api/model.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,15 +66,46 @@ export function executeModel(props) {
filtersLogicalOperator
};

// API supports multiple filters, we apply it only to geoColumn
const spatialFilters = spatialFilter
? {
[source.geoColumn ? source.geoColumn : DEFAULT_GEO_COLUMN]: spatialFilter
let spatialFilters;
if (spatialFilter) {
let spatialDataType = source.spatialDataType;
let spatialDataColumn = source.spatialDataColumn;

if (!spatialDataType || !spatialDataColumn) {
if (source.geoColumn) {
const parsedGeoColumn = source.geoColumn ? source.geoColumn.split(':') : [];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (parsedGeoColumn.length === 2) {
spatialDataType = parsedGeoColumn[0];
spatialDataColumn = parsedGeoColumn[1];
} else if (parsedGeoColumn.length === 1) {
spatialDataColumn = parsedGeoColumn[0] || DEFAULT_GEO_COLUMN;
spatialDataType = 'geo';
}
Comment on lines +80 to +83
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

if (spatialDataType === 'geom') {
// fallback if for some reason someone provided old `geom:whatever`
spatialDataType = 'geo';
}
} else {
spatialDataType = 'geo';
spatialDataColumn = DEFAULT_GEO_COLUMN;
}
: undefined;
}

// API supports multiple filters, we apply it only to geometry column or spatialDataColumn
spatialFilters = spatialFilter
? {
[spatialDataColumn]: spatialFilter
}
: undefined;

if (spatialFilters) {
queryParams.spatialFilters = JSON.stringify(spatialFilters);
queryParams.spatialDataType = spatialDataType;
if (spatialDataType !== 'geo') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

if (source.spatialFiltersResolution !== undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

queryParams.spatialFiltersResolution = source.spatialFiltersResolution;
}
queryParams.spatialFiltersMode = source.spatialFiltersMode || 'intersects';
}
}

const urlWithSearchParams = url + '?' + new URLSearchParams(queryParams).toString();
Expand Down
4 changes: 4 additions & 0 deletions packages/react-api/src/types.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@ export type SourceProps = {
type: MapTypesType['QUERY'] | MapTypesType['TABLE'] | MapTypesType['TILESET'];
connection: string;
geoColumn?: string;
dataResoultion?: number;
zbigg marked this conversation as resolved.
Show resolved Hide resolved
spatialDataType?: string;
spatialDataColumn?: string;
spatialFiltersResolution?: number;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@zbigg zbigg Sep 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

@zbigg zbigg Sep 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok — sounds good to me!

aggregationExp?: string;
credentials?: Credentials;
queryParameters?: QueryParameters;
Expand Down
10 changes: 10 additions & 0 deletions packages/react-redux/src/slices/cartoSlice.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@zbigg zbigg Sep 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* @param {number=} data.spatialFiltersResolution - spatial filters resolution for spatial index data.
* @param {string=} data.aggregationExp - (optional) for spatial index data.
* @param {string=} data.provider - (optional) type of the data warehouse.
*/
Expand All @@ -217,6 +219,10 @@ export const addSource = ({
filtersLogicalOperator = FiltersLogicalOperators.AND,
queryParameters = [],
geoColumn,
spatialDataType,
spatialDataColumn,
dataResolution,
spatialFiltersResolution,
aggregationExp,
provider
}) => ({
Expand All @@ -231,6 +237,10 @@ export const addSource = ({
filtersLogicalOperator,
queryParameters,
geoColumn,
dataResolution,
spatialDataType,
spatialDataColumn,
spatialFiltersResolution,
aggregationExp,
provider
}
Expand Down
48 changes: 46 additions & 2 deletions packages/react-widgets/src/hooks/useWidgetFetch.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,19 @@ export function selectGeometryToIntersect(global, viewport, spatialFilter) {
}
}

// 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));
}

Comment on lines +60 to +72
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yes – that does sound better. :)

export default function useWidgetFetch(
modelFn,
{
Expand Down Expand Up @@ -85,6 +98,7 @@ export default function useWidgetFetch(
);

const viewport = useSelector(selectViewport);
const viewState = useSelector((state) => state.carto.viewState);
const spatialFilter = useSelector((state) =>
selectValidSpatialFilter(state, dataSource)
);
Expand All @@ -93,6 +107,36 @@ export default function useWidgetFetch(
[global, viewport, spatialFilter]
);

const source2 = useMemo(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (
!source ||
!geometryToIntersect ||
!source.dataResolution ||
source.spatialDataType === 'geo'
) {
return source;
}

if (source.spatialDataType === 'h3') {
const hexagonResolution = getHexagonResolution(
{ zoom: viewState.zoom, latitude: viewState.latitude },
source.dataResolution
);
return {
...source,
spatialFiltersResolution: Math.min(source.dataResolution, hexagonResolution)
};
}
if (source.spatialDataType === 'quadbin') {
const quadsResolution = viewState.zoom;
return {
...source,
spatialFiltersResolution: Math.min(source.dataResolution, quadsResolution)
};
}
return source;
}, [geometryToIntersect, source, viewState.zoom, viewState.latitude]);

useCustomCompareEffect(
() => {
let outdated = false;
Expand All @@ -106,7 +150,7 @@ export default function useWidgetFetch(
onStateChange?.({ state: WidgetStateType.Loading });

modelFn({
source,
source: source2,
...params,
global,
remoteCalculation,
Expand Down Expand Up @@ -141,7 +185,7 @@ export default function useWidgetFetch(
},
[
params,
source,
source2,
onError,
isSourceReady,
global,
Expand Down
8 changes: 1 addition & 7 deletions packages/react-widgets/src/models/utils.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,4 @@
import {
AggregationTypes,
getSpatialIndexFromGeoColumn,
_filtersToSQL,
Provider
} from '@carto/react-core';
import { AggregationTypes, _filtersToSQL, Provider } from '@carto/react-core';
import { FullyQualifiedName } from './fqn';
import { MAP_TYPES, API_VERSIONS } from '@carto/react-api';

Expand All @@ -14,7 +9,6 @@ export function isRemoteCalculationSupported(props) {
source &&
source.type !== MAP_TYPES.TILESET &&
source.credentials.apiVersion !== API_VERSIONS.V2 &&
!(source.geoColumn && getSpatialIndexFromGeoColumn(source.geoColumn)) &&
source.provider !== 'databricks'
);
}
Expand Down
Loading