-
Notifications
You must be signed in to change notification settings - Fork 16
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
Changes from 6 commits
4eb8aab
1975cc3
f05b84c
14cad2d
cb289a9
cdeb4cf
debeec3
c5e9740
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(':') : []; | ||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could There was a problem hiding this comment. Choose a reason for hiding this commentThe 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):
|
||
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') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see |
||
if (source.spatialFiltersResolution !== undefined) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Out of curiosity: Could There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was thinking more about something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't like |
||
queryParams.spatialFiltersResolution = source.spatialFiltersResolution; | ||
} | ||
queryParams.spatialFiltersMode = source.spatialFiltersMode || 'intersects'; | ||
} | ||
} | ||
|
||
const urlWithSearchParams = url + '?' + new URLSearchParams(queryParams).toString(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think, creating another slightly different symbol would be confusing, so i would leave it as it is.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, platform APIs already spell this as @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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok — sounds good to me! |
||
aggregationExp?: string; | ||
credentials?: Credentials; | ||
queryParameters?: QueryParameters; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this connected to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't understand connection "well" enough yet.
To apply filters, we need to apply some resolution that is not bigger than The idea is that we allow users to provide two:
That's why we have this There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
*/ | ||
|
@@ -217,6 +219,10 @@ export const addSource = ({ | |
filtersLogicalOperator = FiltersLogicalOperators.AND, | ||
queryParameters = [], | ||
geoColumn, | ||
spatialDataType, | ||
spatialDataColumn, | ||
dataResolution, | ||
spatialFiltersResolution, | ||
aggregationExp, | ||
provider | ||
}) => ({ | ||
|
@@ -231,6 +237,10 @@ export const addSource = ({ | |
filtersLogicalOperator, | ||
queryParameters, | ||
geoColumn, | ||
dataResolution, | ||
spatialDataType, | ||
spatialDataColumn, | ||
spatialFiltersResolution, | ||
aggregationExp, | ||
provider | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, yes – that does sound better. :) |
||
export default function useWidgetFetch( | ||
modelFn, | ||
{ | ||
|
@@ -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) | ||
); | ||
|
@@ -93,6 +107,36 @@ export default function useWidgetFetch( | |
[global, viewport, spatialFilter] | ||
); | ||
|
||
const source2 = useMemo(() => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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; | ||
|
@@ -106,7 +150,7 @@ export default function useWidgetFetch( | |
onStateChange?.({ state: WidgetStateType.Loading }); | ||
|
||
modelFn({ | ||
source, | ||
source: source2, | ||
...params, | ||
global, | ||
remoteCalculation, | ||
|
@@ -141,7 +185,7 @@ export default function useWidgetFetch( | |
}, | ||
[ | ||
params, | ||
source, | ||
source2, | ||
onError, | ||
isSourceReady, | ||
global, | ||
|
There was a problem hiding this comment.
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?