Skip to content

Conversation

@binarycat0
Copy link
Contributor

@binarycat0 binarycat0 commented Aug 28, 2025

org.apache.polaris.persistence.relational.jdbc.JdbcBasePersistenceImpl.listEntities

  • Reduced Column Selection: Only 6 columns instead of 16
  • Eliminated Object Creation Overhead: Direct conversion to EntityNameLookupRecord without intermediate PolarisBaseEntity
  • minor refactoring

Issue: #2352


Query EXPLAIN plan shown in the comment:
#2465 (comment)

Follow-ups

…jdbc.JdbcBasePersistenceImpl.listEntities

- Reduced Column Selection: Only 6 columns instead of 16
- Eliminated Object Creation Overhead: Direct conversion to EntityNameLookupRecord without intermediate PolarisBaseEntity
- Database Index Optimization: Covering index enables index-only scans
@github-project-automation github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Aug 28, 2025
@binarycat0 binarycat0 marked this pull request as ready for review August 28, 2025 16:11
@singhpk234
Copy link
Contributor

Thank you for this change @binarycat0 do you have some perf numbers on how much this benefits compared to existing

@binarycat0
Copy link
Contributor Author

@singhpk234 Hello! Good question. I did not do any performance measurements, but potencially it should help reduce ram and cpu consumption. Could you share a link/clue how I can perform perf testing and compare previous and the current solution?

Copy link
Contributor

@dimas-b dimas-b left a comment

Choose a reason for hiding this comment

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

LGTM overall 👍

Re: perf. testing, from my side, I'd ask for a test that progressively adds entities and shows various API response times as a function of time (or equivalently of the number of entities).

The hardware does not have to be very fast. The point is to compare old code with new code on the same hardware.

@binarycat0

This comment was marked as outdated.

@binarycat0
Copy link
Contributor Author

@singhpk234 My colleagues and I conducted several different experiments and we didn't notice significant improvement on the API side. This is most likely primarily dependent on EntityCache, which greatly improves performance in the current implementation.
Therefore, I can say that the current changes don't significantly improve API performance, but the additional DB INDEX improves SQL experience and reduces query execution time.
Also, in my tests I didn't notice a difference when measuring EXPLAIN PLAN for (select *) versus (select colA, colB...)

@binarycat0 binarycat0 requested a review from dimas-b September 1, 2025 14:32
@binarycat0
Copy link
Contributor Author

Test ENV

OS: 24.5.0 Darwin Kernel Version 24.5.0: Tue Apr 22 19:53:27 PDT 2025; root:xnu-11417.121.6~2/RELEASE_ARM64_T6041 arm64
Storage: Docker image postgres:17
Rows in total = 899040.
Expected rows result = 200005.

Results

Main build version

EXPLAIN ANALYZE
(SELECT id, catalog_id, parent_id, type_code, name, entity_version, sub_type_code, create_timestamp, drop_timestamp, purge_timestamp, to_purge_timestamp, last_update_timestamp, properties, internal_properties, grant_records_version, location_without_scheme
 FROM POLARIS_SCHEMA.ENTITIES
 WHERE catalog_id = 6911759911282783572 AND sub_type_code = 0 AND realm_id = 'POLARIS' AND parent_id = 6911759911282783572 AND type_code = 6);
Bitmap Heap Scan on entities  (cost=3960.68..22260.05 rows=44861 width=140) (actual time=16.287..43.851 rows=200005 loops=1)
  Recheck Cond: ((realm_id = 'POLARIS'::text) AND (catalog_id = '6911759911282783572'::bigint) AND (parent_id = '6911759911282783572'::bigint) AND (type_code = 6))
  Filter: (sub_type_code = 0)
  Heap Blocks: exact=3848
  ->  Bitmap Index Scan on constraint_name  (cost=0.00..3949.47 rows=44861 width=0) (actual time=15.792..15.792 rows=200005 loops=1)
        Index Cond: ((realm_id = 'POLARIS'::text) AND (catalog_id = '6911759911282783572'::bigint) AND (parent_id = '6911759911282783572'::bigint) AND (type_code = 6))
Planning Time: 0.239 ms
Execution Time: 49.253 ms

New build version

CREATE INDEX IF NOT EXISTS idx_entities_lookup
    ON entities USING btree (realm_id, catalog_id, parent_id, type_code, sub_type_code)
    INCLUDE (id, name);
EXPLAIN ANALYZE
(SELECT id, catalog_id, parent_id, type_code, name, sub_type_code 
 FROM POLARIS_SCHEMA.ENTITIES
 WHERE catalog_id = 6911759911282783572 AND sub_type_code = 0 AND realm_id = 'POLARIS' AND parent_id = 6911759911282783572 AND type_code = 6);
Index Only Scan using idx_entities_lookup on entities  (cost=0.42..2975.89 rows=42744 width=50) (actual time=0.030..29.235 rows=200005 loops=1)
  Index Cond: ((realm_id = 'POLARIS'::text) AND (catalog_id = '6911759911282783572'::bigint) AND (parent_id = '6911759911282783572'::bigint) AND (type_code = 6) AND (sub_type_code = 0))
  Heap Fetches: 0
Planning Time: 0.089 ms
Execution Time: 35.317 ms

Conclusion

Using Index Only Scan instead of Bitmap Heap Scan helps:

  • reduce query execution time up to 28%;
  • reducing query cost up to 87%;

cc: @dimas-b @singhpk234

Copy link
Contributor

@XN137 XN137 left a comment

Choose a reason for hiding this comment

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

thanks for working on this, left some minor comments but looks good overall now.

imo since this addresses an existing TODO it shouldnt be strictly required to demonstrate elaborate perf numbers.

what we would need though is a comparison of the postgres explain plan before and after these changes for operations that use PolarisMetaStoreManager.listEntities i.e. listing tables below a parent namespace.
previously is should be an inefficient "table scan" where-as now it is an index-only scan ...
whether that shows an immediately measurable performance improvements depends on a lot of details about benchmark design and postgres internals.

adutra
adutra previously approved these changes Sep 2, 2025
@github-project-automation github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Sep 2, 2025
@dimas-b
Copy link
Contributor

dimas-b commented Sep 2, 2025

Thanks for the perf. update @binarycat0 ! However, the execution times are too small and the sample size is small too (I'm afraid) so the perf. numbers are not decisive, IMHO 😅

At the same time the new index will have some storage and maintenance overhead.

Let's open a dev ML discussion about this and see what people who use Polaris with Postgres think.

The java code changes LGTM 👍

Copy link
Contributor

@dimas-b dimas-b left a comment

Choose a reason for hiding this comment

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

These java code change look like a nice improvement to me.

As for the new index (that was in the earlier revisions of this PR). Let's decide on that based on dev ML discussion.

@dimas-b
Copy link
Contributor

dimas-b commented Sep 2, 2025

@binarycat0 : please remove Database Index Optimization: Covering index enables index-only scans from the description as the new index is no longer in this PR 😅

@dimas-b dimas-b merged commit f6bcbd2 into apache:main Sep 3, 2025
12 of 13 checks passed
@github-project-automation github-project-automation bot moved this from Ready to merge to Done in Basic Kanban Board Sep 3, 2025
snazy added a commit to snazy/polaris that referenced this pull request Nov 20, 2025
* Integration tests for Catalog Federation (apache#2344)

Adds a Junit5 integration test for catalog federation.

* Fix merge conflict in CatalogFederationIntegrationTest (apache#2420)

apache#2344 added a new test for catalog federation, but it looks like an undetected conflict with concurrent changes related to authentication have broken the test in main.

* chore(deps): update registry.access.redhat.com/ubi9/openjdk-21-runtime docker tag to v1.23-6.1755674729 (apache#2416)

* 2334 (apache#2427)

* Fix TableIdentifier in TaskFileIOSupplier (apache#2304)

we cant just convert a `TaskEntity` to a `IcebergTableLikeEntity` as the
`getTableIdentifier()` method will not return a correct value by using
the name of the task and its parent namespace (which is empty?).

task handlers instead need to pass in the `TableIdentifier` that they
already inferred via `TaskEntity.readData`.

* Fix NPE in CreateCatalog (apache#2435)

* Doc fix: Access control page update (apache#2424)

* 2418

* 2418

* fix(deps): update dependency software.amazon.awssdk:bom to v2.32.29 (apache#2443)

* Optimize PolicyCatalog.listPolicies (apache#2370)

this is a follow-up to apache#2290

the optimization is to use `listEntities` instead of `loadEntities` when
there is `policyType` filter to apply

* Add PolarisDiagnostics field to BaseMetaStoreManager (apache#2381)

* Add PolarisDiagnostics field to BaseMetaStoreManager

the ultimate goal is removing the `PolarisCallContext` parameter from every
`PolarisMetaStoreManager` interface method, so we make steps towards
reducing its usage first.

* Add feature flag to disallow custom S3 endpoints (apache#2442)

* Add new realm-level flag: `ALLOW_SETTING_S3_ENDPOINTS` (default: true)

* Enforce in `PolarisServiceImpl.validateStorageConfig()`

Fixes apache#2436

* Deprecate ActiveRolesProvider for removal (apache#2404)

* Client: fix openapi verbose output, remove doc generate, and skip test generations (apache#2439)

* Fix various issue in client code generation

* Use logger instead of print

* Add back exclude on __pycache__ as CI is not via Makefile

* Add back exclude on __pycache__ as CI is not via Makefile

* Add user principal tag in metrics (apache#2445)

* Added API change to enable tag

* Added test

* Added production readiness check

* fix(deps): update dependency io.opentelemetry.semconv:opentelemetry-semconv to v1.36.0 (apache#2454)

* fix(deps): update dependency com.google.cloud:google-cloud-storage-bom to v2.56.0 (apache#2447)

* fix(deps): update dependency gradle.plugin.org.jetbrains.gradle.plugin.idea-ext:gradle-idea-ext to v1.3 (apache#2428)

* Build: Make jandex dependency used for index generation managed (apache#2431)

Also allows specifying the jandex index version for the build.

This is a preparation step contributing to apache#2204, once a jandex fix for reproducible builds is available.

Co-authored-by: Alexandre Dutra <adutra@apache.org>

* Built: improve reproducible archive files (apache#2432)

As part of the effort for apache#2204, this change fixes a few aspects around reproducible builds:

Some Gradle projects produce archive files, but don't get the necessary Gradle archive-tasks settings applied: one not-published project but also the tarball&zip of the distribution. This change moves the logic to the new build-plugin `polaris-reproducible`.

Another change is to have some Quarkus generated jar files adhere to the same conventions, which are constant timestamps for the zip entries and a deterministic order of the entries. That's sadly not a full fix, as the classes that are generated or instumented by Quarkus differ in each build.

Contributes to apache#2204

* Remove commons-lang3 dependency (apache#2456)

outside of tests we can replace the functionality with jdk11 and guava.
also stop using `org.assertj.core.util` as its a non-public api.

* add refresh credentials property to loadTableResult (apache#2341)

* add refresh credentials property to loadTableResult

* IcebergCatalogAdapterTest: Added test to ensure refresh credentials endpoint is included

* delegate refresh credential endpoint configuration to storage integration

* GCP: Add refresh credential properties

* fix(deps): update dependency io.opentelemetry.semconv:opentelemetry-semconv to v1.37.0 (apache#2458)

* Add Delegator to all API Implementations (apache#2434)

Per the Dev ML, implements the Delegator pattern to add Events instrumentation to all Polaris APIs.

* Prefer java.util.Base64 over commons-codec (apache#2463)

`java.util.Base64` is available since java8 and we are already using it
in a few other spots.

in a follow-up we might be able to get rid of our `commons-codec` dependency
completely.

* Service: Move tests to the right package (apache#2469)

* Update versions in runtime LICENSE and NOTICE (apache#2468)

* fix(deps): update dependency com.adobe.testing:s3mock-testcontainers to v4.8.0 (apache#2475)

* fix(deps): update dependency com.gradleup.shadow:shadow-gradle-plugin to v9.1.0 (apache#2476)

* Service: Remove hadoop-common from polaris-runtime-service (apache#2462)

* Service: Always validate allowed locations from Storage Config (apache#2473)

* Add Community Sync Meeting 20250828 (apache#2477)

* Update dependency software.amazon.awssdk:bom to v2.33.0 (apache#2483)

* Remove PolarisCallContext.getDiagServices (apache#2415)

* Remove PolarisCallContext.getDiagServices usage

* Remove diagnostics from PolarisCallContext

* Feature: Expose resetCredentials via a new reset api to allow root user to reset credentials for an existing principal with custom values  (apache#2197)

* Add type-check to PolarisEntity subclass ctors (apache#2302)

currently one can freely "cast" any `PolarisEntity` to a more
specific type via their constructors.

this can lead to subtle bugs like we fixed in
a29f800

by adding type checks we discover a few more places where we need to be
more careful about how we construct new or handle existing entities.

note that we can add a check for `PolarisEntitySubType` in a followup,
but it requires more fixes currently.

* Fix CI (apache#2489)

Fix undetected merge conflict after apache#2197 + apache#2415 + apache#2434

* Use local diagnostics in TransactionWorkspaceMetaStoreManager

* Add resetCredentials to PolarisPrincipalsEventServiceDelegator

* Core: Prevent AIOOBE for negative codes in PolarisEntityType, PolarisPrivilege, ReturnStatus (apache#2490)

* feat(idgen): Start Implementation of NoSQL with the ID Generation Framework (apache#2131)

Create an ID Generation Framework.

Related to apache#650 & apache#844

Co-authored-by: Robert Stupp <snazy@snazy.de>
Co-authored-by: Dmitri Bourlatchkov <dmitri.bourlatchkov@gmail.com>

* perf(refactor): optimizing JdbcBasePersistenceImpl.listEntities (apache#2465)

- Reduced Column Selection: Only 6 columns instead of 16

- Eliminated Object Creation Overhead: Direct conversion to EntityNameLookupRecord without intermediate PolarisBaseEntity

* Add Polaris Events to Persistence (apache#1844)

* AWS CloudWatch Event Sink Implementation (apache#1965)

* Fix failing CI (apache#2498)

* Update actions/stale digest to 3a9db7e (apache#2499)

* Core: Prevent AIOOBE for negative policy codes in PredefinedPolicyType (apache#2486)

* Service: Add location tests for views (apache#2496)

* Update docker.io/jaegertracing/all-in-one Docker tag to v1.73.0 (apache#2500)

* Update dependency io.netty:netty-codec-http2 to v4.2.5.Final (apache#2495)

* Update actions/setup-python action to v6 (apache#2502)

* Update the Release Guide about the Helm Chart package (apache#2179)

* Update the Release Guide about the Helm Chart package

* Update release-guide.md

Co-authored-by: Pierre Laporte <pierre@pingtimeout.fr>

* Add missing commit message

* Whitespace

* Use Helm GPG plugin to sign the Helm chart

* Fix directories during Helm chart copy to SVN

* Add Helm index to SVN

* Use long name for svn checkout

* Ensure the Helm index is updated after the chart is moved to SVN dist release

* Do not publish any Docker image before the vote succeeds

* Typos

* Revert "Do not publish any Docker image before the vote succeeds"

This reverts commit 5617e65.

* Don't mention Helm values.yaml in the release guide as it doesn't contain version details

---------

Co-authored-by: Pierre Laporte <pierre@pingtimeout.fr>

* Update dependency com.azure:azure-sdk-bom to v1.2.38 (apache#2503)

* Update registry.access.redhat.com/ubi9/openjdk-21-runtime Docker tag to v1.23-6.1756793420 (apache#2504)

* Remove commons-codec dependency (apache#2474)

follow-up to f8ad77a

we can simply use guava instead and eliminate the extra dependency

* CLI: Remove SCRIPT_DIR and default config location to user home (apache#2448)

* Remove readInternalProperties helpers (apache#2506)

the functionality is already provided by the `PrincipalEntity`

* Add Events for Generic Table APIs (apache#2481)


This PR adds the Events instrumentation for the Generic Tables Service APIs, surrounding the default delegated call to the business logic APIs.

* Disable custom namespace locations (apache#2422)

When we create a namespace or alter its location, we must confirm that this location is within the parent location. This PR introduces introduces a check similar to the one we have for tables, where custom locations are prohibited by default. This functionality is gated behind a new behavior change flag `ALLOW_NAMESPACE_CUSTOM_LOCATION`. In addition to allowing us to revert to the old behavior, this flag allows some tests relying on arbitrarily-located namespaces to pass (such as those from upstream Iceberg).

Fixes: apache#2417

* fix for IcebergAllowedLocationTest (apache#2511)

* Remove unused config from SparkSessionBuilder (apache#2512)

Tests pass without it.

* Add Events for Policy Service APIs (apache#2479)

* Remove PolarisTestMetaStoreManager.jsonNode helper (apache#2513)

* Update dependency software.amazon.awssdk:bom to v2.33.4 (apache#2517)

* Update dependency com.nimbusds:nimbus-jose-jwt to v10.5 (apache#2514)

* Update dependency io.opentelemetry:opentelemetry-bom to v1.54.0 (apache#2515)

* Update dependency io.micrometer:micrometer-bom to v1.15.4 (apache#2519)

* Port missed OSS change

* NoSQL: adopt to updated test packages

* NoSQL: adapt to removed PolarisDiagnostics param

* NoSQL: fix libs.versions.toml

* NoSQL: include jandex plugin related changes from OSS

* NoSQL: changes for delete/set principal client-ID+secret

* Last merged commit c6176dc

---------

Co-authored-by: Pooja Nilangekar <poojan@umd.edu>
Co-authored-by: Eric Maynard <eric.maynard+oss@snowflake.com>
Co-authored-by: Mend Renovate <bot@renovateapp.com>
Co-authored-by: Yong Zheng <yongzheng0809@gmail.com>
Co-authored-by: Christopher Lambert <xn137@gmx.de>
Co-authored-by: Honah (Jonas) J. <honahx@apache.org>
Co-authored-by: Dmitri Bourlatchkov <dmitri.bourlatchkov@gmail.com>
Co-authored-by: Alexandre Dutra <adutra@apache.org>
Co-authored-by: fivetran-kostaszoumpatianos <kostas.zoumpatianos@fivetran.com>
Co-authored-by: Jason <jasonf20@gmail.com>
Co-authored-by: Adnan Hemani <adnan.h@berkeley.edu>
Co-authored-by: Yufei Gu <yufei@apache.org>
Co-authored-by: JB Onofré <jbonofre@apache.org>
Co-authored-by: fivetran-arunsuri <103934371+fivetran-arunsuri@users.noreply.github.com>
Co-authored-by: Adam Christian <105929021+adam-christian-software@users.noreply.github.com>
Co-authored-by: Artur Rakhmatulin <artur.rakhmatulin@gmail.com>
Co-authored-by: Pierre Laporte <pierre@pingtimeout.fr>
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.

5 participants