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

fix: PerfAPI codegen supports reading a feed that has a subset of the schema #654

Merged
merged 1 commit into from
Jan 8, 2024

Conversation

eduardoramirez
Copy link
Contributor

@eduardoramirez eduardoramirez commented Dec 21, 2023

Prevents the generated Perf API from throwing exceptions when used against an older version of the feed.

repro

  • add a new field to schema
  • re-generate the client
  • populate the feed in test, test works
  • release code to prod but don't update feed in prod yet. PerfAPI throws ArrayOutOfBounds on getter for field that isn't populated in feed but exists in schema

builder.append(" return typeAccess.readInt(ordinal(ref), fieldIdx[" + fieldIdx + "]);\n");
builder.append(" }\n\n");

builder.append(" public Integer get" + HollowCodeGenerationUtils.upperFirstChar(fieldName) + "Boxed(long ref) {\n");
builder.append(" if(fieldIdx[" + fieldIdx + "] == -1)\n");
builder.append(" return null;\n");
Copy link
Contributor

@Sunjeet Sunjeet Jan 3, 2024

Choose a reason for hiding this comment

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

Shouldn't these (Integer, Long, Float, and Double) return Integer.MIN_VALUE/Float.NaN etc.? Just going by the classic generated API-
In LongTypeAPI.java,

public Long getValueBoxed(int ordinal) {
        long l;
        if(fieldIndex[0] == -1) {
            l = missingDataHandler().handleLong("Long", ordinal, "value");
        ...

Copy link
Contributor Author

@eduardoramirez eduardoramirez Jan 3, 2024

Choose a reason for hiding this comment

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

hmm interesting... in the actual generated code for something like MidInsightsDelegateLookupImpl the getter for primitive and boxed long looks like this:

    public long getMovieId(int ordinal) {
        ordinal = typeAPI.getMovieIdOrdinal(ordinal);
        return ordinal == -1 ? Long.MIN_VALUE : typeAPI.getAPI().getLongTypeAPI().getValue(ordinal);
    }

    public Long getMovieIdBoxed(int ordinal) {
        ordinal = typeAPI.getMovieIdOrdinal(ordinal);
        return ordinal == -1 ? null : typeAPI.getAPI().getLongTypeAPI().getValueBoxed(ordinal);
    }

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm yeah looks like the defaults aren't consistently applied in the generated api

Copy link
Contributor

Choose a reason for hiding this comment

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

Lets ship this improvement for now, I've tracked the fix for consistently applying defaults in this issue

builder.append(" return Boolean.TRUE.equals(typeAccess.readBoolean(ordinal(ref), fieldIdx[" + fieldIdx + "]));\n");
builder.append(" }\n\n");

builder.append(" public Boolean get" + HollowCodeGenerationUtils.upperFirstChar(fieldName) + "Boxed(long ref) {\n");
builder.append(" if(fieldIdx[" + fieldIdx + "] == -1)\n");
builder.append(" return null;\n");
Copy link
Contributor

Choose a reason for hiding this comment

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

For Boolean this seems to match what the classic API would return

@eduardoramirez eduardoramirez merged commit 13674ed into master Jan 8, 2024
2 checks passed
@eduardoramirez eduardoramirez deleted the perf-api-compat branch January 8, 2024 17:35
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.

2 participants