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

get_SDA_property: CTEs for SQLite compatibility and query readability #381

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

brownag
Copy link
Member

@brownag brownag commented Mar 10, 2025

This PR replaces SELECT ... INTO ... SQL syntax with Common Table Expressions (CTEs) in get_SDA_property(). Temporary tables are used in the dominant component (numeric) and weighted average aggregation methods.

  • implement CTEs for Weighted Average aggregation (includes dominant component, numeric)
  • implement CTEs for MIN/MAX aggregation
  • Benchmark against gNATSGO and SSURGOPortal output for a few other properties (e.g. available water capacity)

This change has the benefit of increasing the compatibility of get_SDA_property() methods with SQLite and other dialects. I've done some benchmarking of old vs. new as well as against SSURGOPortal output. We have 1:1 parity across tools and dialects, and all existing soilDB tests are passing with the new code.

In this PR I've also built out the infrastructure that will make it relatively easy implement this approach for the other areas I identified in #250.

Before merging I will be testing this across the country to verify there aren't edge cases when querying multiple properties with very different patterns of missing values, and adding additional tests if needed to make sure that type of edge-case functionality is fully tested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant