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

ISSUE-6612 FIX: parse long field in GenricJsonRecord (#6612) #6622

Merged
merged 2 commits into from Apr 5, 2020

Conversation

lynnmatrix
Copy link
Contributor

Fixes #6612

Motivation

If message sent in json schema, long field will be decoded as int if its value below Integer.MAX_VALUE, other wise decoded as string.
For example, the json message below:

{
    "timestamp": 1585204833128
}

will be decoded as

{
    "timestamp": "1585204833128"
}

Modifications

Add field type check in GenericJsonRecord

Verifying this change

  • Make sure that the change passes the CI checks.
  • Unit test added

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): no
  • The public API: no
  • The schema: don't know
  • The default values of configurations: no
  • The wire protocol: no
  • The rest endpoints: no
  • The admin cli options: no
  • Anything that affects deployment: no

Documentation

  • Does this pull request introduce a new feature? no

@lynnmatrix
Copy link
Contributor Author

/pulsarbot run-failure-checks

@codelipenghui
Copy link
Contributor

/pulsarbot run-failure-checks

6 similar comments
@codelipenghui
Copy link
Contributor

/pulsarbot run-failure-checks

@lynnmatrix
Copy link
Contributor Author

/pulsarbot run-failure-checks

@jiazhai
Copy link
Member

jiazhai commented Mar 28, 2020

/pulsarbot run-failure-checks

@codelipenghui
Copy link
Contributor

/pulsarbot run-failure-checks

@lynnmatrix
Copy link
Contributor Author

/pulsarbot run-failure-checks

@lynnmatrix
Copy link
Contributor Author

/pulsarbot run-failure-checks

@sijie sijie added the type/bug The PR fixed a bug or issue reported a bug label Mar 30, 2020
@sijie sijie added this to the 2.6.0 milestone Mar 30, 2020
@lynnmatrix
Copy link
Contributor Author

/pulsarbot run-failure-checks

2 similar comments
@lynnmatrix
Copy link
Contributor Author

/pulsarbot run-failure-checks

@lynnmatrix
Copy link
Contributor Author

/pulsarbot run-failure-checks

return fn.asDouble();
} else if (fn.isDouble()) {
return fn.asDouble();
} else if (fn.isNumber()) {
Copy link
Member

Choose a reason for hiding this comment

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

I think the integration tests failed due to this change.

I think we should keep the original behavior and add the following

else if (fn.isLong()) {
    return fn.asLong();
} else if (fn.isNumber()) {
    return fn.numberValue();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if fn is long, then fn.aslong() should equals to fn.numberValue()。
The differences compared to the previous code are:

  1. If fn is BigDecimal, the new code will return BigDecimal rather than double
  2. If fn is BigInteger, the new code will return BigInteger rather than text
  3. If fn is float, the new code will return float rather than double

So i modify the code like:

        } else if (fn.isBoolean()) {
            return fn.asBoolean();
        } else if (fn.isFloatingPointNumber()) {
            return fn.asDouble();
        } else if (fn.isBigInteger()) {
            if (fn.canConvertToLong()) {
                return fn.asLong();
            } else {
                return fn.asText();
            }
        } else if (fn.isNumber()) {
            return fn.numberValue();
        } else {
            return fn.asText();
        }

I am trying to do integration tests locally but it takes too long to download snapshot jars from maven. I'll try again, and if fail again, can I push directly and do ci test?

Copy link
Member

Choose a reason for hiding this comment

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

sounds good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there anything wrong with CI workflow? I reset to commit the one before the first commit of this PR, and it still fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found this pull #6660, and it has been merged.
I should try again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Finally pass the CI

@lynnmatrix
Copy link
Contributor Author

/pulsarbot run-failure-checks

1 similar comment
@jiazhai
Copy link
Member

jiazhai commented Apr 2, 2020

/pulsarbot run-failure-checks

@lynnmatrix
Copy link
Contributor Author

/pulsarbot run-failure-checks

4 similar comments
@lynnmatrix
Copy link
Contributor Author

/pulsarbot run-failure-checks

@lynnmatrix
Copy link
Contributor Author

/pulsarbot run-failure-checks

@lynnmatrix
Copy link
Contributor Author

/pulsarbot run-failure-checks

@lynnmatrix
Copy link
Contributor Author

/pulsarbot run-failure-checks

@lynnmatrix
Copy link
Contributor Author

/pulsarbot run-failure-checks

2 similar comments
@lynnmatrix
Copy link
Contributor Author

/pulsarbot run-failure-checks

@lynnmatrix
Copy link
Contributor Author

/pulsarbot run-failure-checks

@lynnmatrix
Copy link
Contributor Author

/pulsarbot run-failure-checks

@lynnmatrix lynnmatrix force-pushed the fix/json_decode_long branch 2 times, most recently from 03b803d to d4ee5a6 Compare April 3, 2020 18:19
@lynnmatrix
Copy link
Contributor Author

/pulsarbot run-failure-checks

1 similar comment
@lynnmatrix
Copy link
Contributor Author

/pulsarbot run-failure-checks

@lynnmatrix
Copy link
Contributor Author

/pulsarbot run-failure-checks

2 similar comments
@lynnmatrix
Copy link
Contributor Author

/pulsarbot run-failure-checks

@lynnmatrix
Copy link
Contributor Author

/pulsarbot run-failure-checks

@sijie sijie merged commit 1aad3b7 into apache:master Apr 5, 2020
addisonj pushed a commit to instructure/pulsar that referenced this pull request May 7, 2020
…pache#6622)

Fixes apache#6612


### Motivation

If message sent in json schema, long field will be decoded as int if its value below Integer.MAX_VALUE, other wise decoded as string.
For example, the json message below: 
```json
{
    "timestamp": 1585204833128
}
```
will be decoded as 

```json
{
    "timestamp": "1585204833128"
}
```

### Modifications

Add field type check in GenericJsonRecord
jiazhai pushed a commit that referenced this pull request May 8, 2020
Fixes #6612

### Motivation

If message sent in json schema, long field will be decoded as int if its value below Integer.MAX_VALUE, other wise decoded as string.
For example, the json message below:
```json
{
    "timestamp": 1585204833128
}
```
will be decoded as

```json
{
    "timestamp": "1585204833128"
}
```

### Modifications

Add field type check in GenericJsonRecord
(cherry picked from commit 1aad3b7)
huangdx0726 pushed a commit to huangdx0726/pulsar that referenced this pull request Aug 24, 2020
…pache#6622)

Fixes apache#6612


### Motivation

If message sent in json schema, long field will be decoded as int if its value below Integer.MAX_VALUE, other wise decoded as string.
For example, the json message below: 
```json
{
    "timestamp": 1585204833128
}
```
will be decoded as 

```json
{
    "timestamp": "1585204833128"
}
```

### Modifications

Add field type check in GenericJsonRecord
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release/2.5.2 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failed to decode long type fields in json schema message
4 participants