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

Handling circular type definitions #3

Closed
randyzwitch opened this issue Nov 2, 2018 · 8 comments
Closed

Handling circular type definitions #3

randyzwitch opened this issue Nov 2, 2018 · 8 comments

Comments

@randyzwitch
Copy link
Contributor

The Thrift spec allows for circular type definition; this currently throws an error (I presume from top-down parsing of the .thrift file)

/* union */ struct TDatumVal {
  1: i64 int_val,
  2: double real_val,
  3: string str_val,
  4: list<TDatum> arr_val
}

struct TDatum {
  1: TDatumVal val,
  2: bool is_null
}

Result:

> mapdthrift = t_load("src/mapd.thrift")
Error in value[[3L]](cond) : No type found: TDatum, at line 64
@marekjagielski
Copy link
Member

I would check if https://github.com/eleme/thriftpy handles circular types. I am guessing that not. Anyway, I think it would be possible to implement it. I would also start from contributing the change to thriftpy.

@randyzwitch
Copy link
Contributor Author

thriftpy has this as an open issue as well

@randyzwitch
Copy link
Contributor Author

Thriftpy/thriftpy#280

@ethe
Copy link

ethe commented Dec 27, 2018

It has already been solved in thriftpy2, thanks!

@randyzwitch
Copy link
Contributor Author

Thanks for letting us know @ethe!

@marekjagielski How are you generating the R code from the Python code? Is it something I can help with? I'd love to give thriftr another chance, as I'd love to avoid wrapping the Python thrift library using reticulate and just use plain R directly

@marekjagielski
Copy link
Member

@randyzwitch , I am happy that you are willing to contribute. I will be busy next 2 months, so I will not be able to code on thriftr.
There is no automatic generation of R code. I just went line by line and tried to translate the syntax. So this logic has to be added to thriftr:
https://github.com/Thriftpy/thriftpy2/pull/21/files

marekjagielski added a commit that referenced this issue Feb 24, 2019
#3 Handling circular type definitions
@ethe
Copy link

ethe commented Feb 25, 2019

There are some bugs in ThriftPy handling out-of-order definitions, see Thriftpy/thriftpy2#42 and get more information.

@marekjagielski
Copy link
Member

@ethe Thank you!
I will incorporate these changes to thriftr.

marekjagielski added a commit that referenced this issue Mar 3, 2019
issue #3 introduce fix 41 from thriftpy2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants