Makes sense. If there’s a clear plan to have this PR be part of a feature in Polaris or an improvement to one of its APIs, I’m totally supportive of merging this change.
>From your response, it sounds like this is a prerequisite to another PR that you have planned, so I'll be sure to check that out when it's ready. Absent that other PR, I still think it's hard to judge whether #2735 is correctly serving as that prerequisite. Indeed, there have been a few discussions around merging "dead code" into the project (e.g. after #1230) and so I continue to believe that it's best to exercise constraint when merging code with no immediately apparent utility. --EM On Tue, Oct 14, 2025 at 10:21 AM Michael Collado <[email protected]> wrote: > > Instead, the approach on the PR seems to sort of "backdoor" in an API > change without an ML thread by first making the persistence-related changes > that optimize that API > > I'm still not sure what that means. I asked in the PR what API you're > suggesting and you never answered. Do you mean a new REST API? Or a > PolarisMetaStoreManager change? I have no new REST endpoints in mind at > this point and I don't think a REST endpoint is necessary to be able to > take advantage of this information. > > > You gave the example of the task interface, but there have been several > proposals to rework the task system exactly because it's not very > well-engineered. > > Whether the task execution framework is well-engineered is irrelevant. It > is a fact that parts of the service operate based on data stored in > persistence that isn't shared over an externally facing API. This doesn't > seem like a controversial topic to me. > > > In light of this its current state #2735 seems to be a sort of > re-imagining of #433, but without any of the tangible benefits or > measurements around perf. > > I think everyone agreed over the mailing list that the intended benefits of > #433 are desirable, even if the implementation itself lacked rollback > safety. #2735 is, in fact, a prerequisite to an alternative approach that > we discussed several months ago, as is #2508. In fact, I'd really like to > experiment with real numbers on a real deployment of Polaris to have a > safe, efficient, performant implementation of TableMetadata in persistence, > but that isn't ready yet. I'm not a fan of big-bang releases or giant PRs > and I think small, easily digestible, easily rollback-able changes are > safer and make for a better product. And given that both #2735 and #2508 > offer benefits that can be taken advantage of even without the full > implementation of #433, I don't see a reason to bundle all of these changes > together. > > So I ask again, as I asked on the PR, what is the technical justification > for the veto? If there's a specific concern, like rollback safety or memory > usage, I'd be happy to hear it and make the appropriate changes. If the > concern is simply that this doesn't fully complete the implementation of > #433, I don't think that qualifies as a valid justification. > > Mike > > On Mon, Oct 13, 2025 at 11:28 AM Eric Maynard <[email protected]> > wrote: > > > Hi Michael, > > > > I see the request on that PR as quite specific: > > > > > Wouldn't it make sense to design the API first before making > persistence > > changes? > > > > Instead, the approach on the PR seems to sort of "backdoor" in an API > > change without an ML thread by first making the persistence-related > changes > > that optimize that API. Indeed, when the question was posed about what > the > > tangible benefit of the PR might be, you replied: > > > > > As an example, JB recently brought up working on the UI - it would be > > nice if the UI could print some information about the table without > loading > > the entire metadata > > > > Presumably the UI would do this through some API, no? Or else how would > > #2735 facilitate the UI doing this? However, you also commented: > > > > > What API? > > > Not every field stored in the persistence layer serves a public-facing > > API > > > > In light of this its current state #2735 seems to be a sort of > re-imagining > > of #433, but without any of the tangible benefits or measurements around > > perf. As it stands, I don't see how the project benefits from merging > #2735 > > and tried to probe this with my questions on the PR. This question seems > to > > remain unaddressed. > > > > You did comment that the PR being blocked implied a question about > whether > > it was "unsafe or incorrect", which is confusing to me -- one could > propose > > adding to Polaris some class that correctly and safely implements image > > rotation, but without a viable use-case for that class I would hope we > > wouldn't merge that code. > > > > As to the other recent comment that it could be useful to add things to > > persistence without any API exposing them, I'm not so sure about that. > You > > gave the example of the task interface, but there have been several > > proposals to rework the task system exactly because it's not very > > well-engineered. My understanding has historically been that the > metastore > > exists in service of the catalog service, which is exposed over a REST > API. > > Happy to discuss this more. > > > > In short, if there's no benefit to merging #2735, I don't see why we > would > > rush into merging it. If our intent is to add some API that this PR might > > optimize, why not design, discuss, and implement this API first before > > going ahead with a premature optimization? > > > > --EM > > > > > > > > On Mon, Oct 13, 2025 at 10:55 AM Michael Collado <[email protected] > > > > wrote: > > > > > Hey folks > > > > > > I got approval on https://github.com/apache/polaris/pull/2735 ten days > > > ago, > > > but a request for changes last Monday without any specific request or > > > objection. I've asked a couple of times what the objection is, but > > haven't > > > heard a response. As far as I can see, there's no technical reason to > > block > > > this change. What's the community stance on this PR? At least three > > people > > > (aside from myself) have suggested that the feature is useful. Does > > anyone > > > feel that there is a technical reason for blocking this PR from > merging? > > If > > > there is a concrete objection, I'm happy to hear it, but if people feel > > > this is generally useful, I'd like to move forward with merging. > > > > > > Mke > > > > > >
