> I do not think we have technical means to force JDBC schema changes on upgrade.
Would a warning under "Upgrade notes" in the changelog be enough? On Fri, Oct 24, 2025 at 5:53 PM Dmitri Bourlatchkov <[email protected]> wrote: > > Hi All, > > From my POV we should flag events as "beta" in 1.2.0 release notes and > drop/re-create events tables on upgrade. > > However, I do not think we have technical means to force JDBC schema > changes on upgrade. At best we can disable event persistence (with ERROR > log messages) if the schema is old and wait for the user to run the admin > tool to apply schema changes. > > Cheers, > Dmitri. > > On Fri, Oct 24, 2025 at 5:26 AM Alexandre Dutra <[email protected]> wrote: > > > Hi all, > > > > Thank you for your input here. Mike and Adnan: I'm fine with numerical > > ids in the database, let's keep using this pattern. > > > > I think the only question remaining is how to handle the SQL schema > > migration. Currently we have an event_type column of type TEXT NOT > > NULL [1]. We would need instead an event_type_code column of type > > INTEGER NOT NULL. > > > > Like Dmitri, I confess I do not recall if the Events API is considered > > beta or production-ready at this point. Also, the events table is > > probably meant to be purged regularly. Do you think it would be OK to > > just drop the existing column and introduce a new one, also > > instructing users to drop and re-create the table when upgrading > > Polaris? > > > > Thanks, > > Alex > > > > [1]: > > https://github.com/apache/polaris/blob/e02bb7171ba7af1aeba224e4d5fd585ea6c88624/persistence/relational-jdbc/src/main/resources/postgres/schema-v3.sql#L129 > > > > On Fri, Oct 24, 2025 at 9:48 AM Adnan Hemani > > <[email protected]> wrote: > > > > > > Hi Alex, > > > > > > I agree with your concerns. Here is what I am recommending: > > > * +1 to Michael’s recommendation of keeping numeric IDs for storage > > purposes. > > > * Waiting on the logical IDs until we have a use case for it. I’m not > > sure the configurations use case is fully defined yet (if you think it is, > > happy to discuss on a different mailing thread so as to not hijack this > > thread). But we should add a new method regardless to the PolarisEvent > > interface: `int typeId()` or something similar - and use that forward in > > storage. > > > > > > Best, > > > Adnan Hemani > > > > > > > On Oct 23, 2025, at 9:23 AM, Michael Collado <[email protected]> > > wrote: > > > > > > > > Totally agree. In fact, other enum types (e.g., PolarisPrivilege, > > > > PolarisEntityType) use numeric ids in storage to decouple naming in > > code > > > > from the values we persist in the database. I'd recommend continuing > > with > > > > that pattern. > > > > > > > > Mike > > > > > > > > On Wed, Oct 22, 2025 at 10:38 AM Dmitri Bourlatchkov <[email protected] > > <mailto:[email protected]>> > > > > wrote: > > > > > > > >> Hi Alex, > > > >> > > > >> +1 to all three counts. > > > >> > > > >> As for the SQL migration script, is event persistence a fully > > supported > > > >> feature or a "beta"... TBH, I do not recall how it was presented to > > users. > > > >> WDYT? > > > >> > > > >> Cheers, > > > >> Dmitri. > > > >> > > > >> On Wed, Oct 22, 2025 at 9:44 AM Alexandre Dutra <[email protected]> > > wrote: > > > >> > > > >>> Hi all, > > > >>> > > > >>> Currently, Polaris manages over 150 event types, but they lack proper > > > >>> identifiers, leading to the problematic use of Java class names to > > > >>> represent the event types. > > > >>> > > > >>> For instance, PolarisPersistenceEventListener relies on Java class > > > >>> names to store event types in the database [1]. Thus, renaming or > > > >>> moving an event class could inadvertently break this listener, or any > > > >>> other listener doing something similar. > > > >>> > > > >>> Moreover, if we were to introduce a configuration to manage event > > > >>> types (which I think we'll have to do eventually), we'd need to use > > > >>> Java class names again, out of better options. Consider a > > hypothetical > > > >>> configuration example that highlights this issue: > > > >>> > > > >>> polaris.event-listener.enabled-event-types=\ > > > >>> > > > >>> > > > >> > > org.apache.polaris.service.events.IcebergRestCatalogEvents.AfterRefreshTableEvent,\ > > > >>> > > > >>> > > > >> > > org.apache.polaris.service.events.IcebergRestCatalogEvents.AfterCommitTableEvent > > > >>> > > > >>> It's obvious that listing event types in such a way is not only > > > >>> verbose, but also cryptic for users. It introduces an unnecessary > > > >>> tight coupling between user-facing configuration and Polaris > > > >>> internals. > > > >>> > > > >>> To address these concerns, I would like to promote event types to > > > >>> first-class citizens in Polaris: > > > >>> > > > >>> 1. Introduce the concept of an event type with a unique, logical ID > > > >>> (e.g., BEFORE_CREATE_CATALOG). This could be enforced through a Java > > > >>> enum for uniqueness. > > > >>> 2. Add a new method, PolarisEventType type(), to the PolarisEvent > > > >>> interface. This is not only informative; it could serve e.g. as a > > type > > > >>> discriminator when serializing events. > > > >>> 3. Replace class names with logical IDs, both in configuration and > > > >>> when storing events in the database. > > > >>> > > > >>> Note: The last item represents a breaking change and may require a > > SQL > > > >>> migration script. > > > >>> > > > >>> I'm curious to hear your thoughts. > > > >>> > > > >>> Thanks, > > > >>> Alex > > > >>> > > > >>> [1]: > > > >>> > > > >> > > https://www.google.com/url?q=https://github.com/apache/polaris/blob/3b4b0169032216d85d48130ad43285f86e886e3c/runtime/service/src/main/java/org/apache/polaris/service/events/listeners/PolarisPersistenceEventListener.java%23L44&source=gmail-imap&ust=1761841473000000&usg=AOvVaw0v18HDrE4mH9C3INPPhtLp > > > > >
