Skip to content

allow opt-in for new unset field semantics on csp.Struct (disc #536) #574

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.

Sign up for GitHub

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

Closed
wants to merge 8 commits into from
Closed

allow opt-in for new unset field semantics on csp.Struct (disc #536) #574

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
195165f
add preliminary unset field semantics (disc #536)
Jul 31, 2025
345f37f
style nits
Aug 13, 2025
4acbb08
revert and simplify optional field interface
Aug 13, 2025
2af2edf
fix minor bugs in validation logic
Aug 13, 2025
685bf73
nodiscard bool on validate
Aug 14, 2025
a8751cf
validate autogen'ed dynamic structs
Aug 14, 2025
6816484
disallow non-defaulted optional fields on strict structs
Aug 14, 2025
2603e73
catch adapter exceptions
Aug 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions cpp/csp/adapters/kafka/KafkaInputAdapter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,8 @@ void KafkaInputAdapter::processMessage( RdKafka::Message* message, bool live, cs
if( m_tickTimestampField )
msgTime = m_tickTimestampField->value<DateTime>(tick.get());

tick.get() -> validate();
Copy link
Collaborator

Choose a reason for hiding this comment

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

we need to be careful on all of the realtime adapters that are now validating. Keep in mind this is happening on a separate thread and an uncaught exception will crash the process.
Just need to make sure all of these validation calls are already under existing try/catch blocks ( some of them may already be )

Copy link
Author

Choose a reason for hiding this comment

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

I added some new changes that hopefully deal with this, though a sanity check is definitely needed. Left some corresponding comments as well.


bool pushLive = shouldPushLive(live, msgTime);
if( shouldProcessMessage( pushLive, msgTime ) )
pushTick(pushLive, msgTime, std::move(tick), batch);
Expand Down
1 change: 1 addition & 0 deletions cpp/csp/adapters/parquet/ParquetReaderColumnAdapter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -520,6 +520,7 @@ void ParquetStructAdapter::dispatchValue( const utils::Symbol *symbol, bool isNu
{
fieldSetter( s );
}
s -> validate();
dispatchedValue = &s;
}

Expand Down
3 changes: 3 additions & 0 deletions cpp/csp/adapters/utils/JSONMessageStructConverter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,8 @@ StructPtr JSONMessageStructConverter::convertJSON( const char * fieldname, const
} );
}

struct_ -> validate();

return struct_;
}

Expand Down Expand Up @@ -251,6 +253,7 @@ csp::StructPtr JSONMessageStructConverter::asStruct( void * bytes, size_t size )
}
);
}
// root struct validation (validate()) deferred to adapter level

return data;
}
Expand Down
1 change: 1 addition & 0 deletions cpp/csp/adapters/websocket/ClientInputAdapter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ void ClientInputAdapter::processMessage( void* c, size_t t, PushBatch* batch )
if( dataType() -> type() == CspType::Type::STRUCT )
{
auto tick = m_converter -> asStruct( c, t );
tick.get() -> validate();
pushTick( std::move(tick), batch );
} else if ( dataType() -> type() == CspType::Type::STRING )
{
Expand Down
3 changes: 2 additions & 1 deletion cpp/csp/cppnodes/baselibimpl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -705,6 +705,7 @@ DECLARE_CPPNODE( struct_fromts )
);
}

out.get() -> validate( );
CSP_OUTPUT( std::move( out ) );
}

Expand Down Expand Up @@ -758,7 +759,7 @@ DECLARE_CPPNODE( struct_collectts )
}
);
}

out.get() -> validate( );
CSP_OUTPUT( std::move( out ) );
}

Expand Down
48 changes: 44 additions & 4 deletions cpp/csp/engine/Struct.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,23 @@
#include <csp/core/System.h>
#include <csp/engine/Struct.h>
#include <algorithm>
#include <ranges>
#include <string>

namespace csp
{

StructField::StructField( CspTypePtr type, const std::string & fieldname,
size_t size, size_t alignment ) :
size_t size, size_t alignment, bool isOptional ) :
m_fieldname( fieldname ),
m_offset( 0 ),
m_size( size ),
m_alignment( alignment ),
m_maskOffset( 0 ),
m_maskBit( 0 ),
m_maskBitMask( 0 ),
m_type( type )
m_type( type ),
m_isOptional( isOptional )
{
}

Expand All @@ -33,8 +36,8 @@ and adjustments required for the hidden fields

*/

StructMeta::StructMeta( const std::string & name, const Fields & fields,
std::shared_ptr<StructMeta> base ) : m_name( name ), m_base( base ), m_fields( fields ),
StructMeta::StructMeta( const std::string & name, const Fields & fields, bool isStrict,
std::shared_ptr<StructMeta> base ) : m_name( name ), m_base( base ), m_isStrict( isStrict ), m_fields( fields ),
m_size( 0 ), m_partialSize( 0 ), m_partialStart( 0 ), m_nativeStart( 0 ), m_basePadding( 0 ),
m_maskLoc( 0 ), m_maskSize( 0 ), m_firstPartialField( 0 ), m_firstNativePartialField( 0 ),
m_isPartialNative( true ), m_isFullyNative( true )
Expand Down Expand Up @@ -494,6 +497,43 @@ void StructMeta::destroy( Struct * s ) const
m_base -> destroy( s );
}

void StructMeta::validate( const Struct * s ) const
{
bool encountered_non_strict = false;

for( const StructMeta * cur = this; cur; cur = cur -> m_base.get() )
{
encountered_non_strict |= !cur -> isStrict();
if( !cur -> isStrict() ) {
continue;
}

// rule 1: a non-strict struct may not inherit (directly or indirectly) from a strict base
if (encountered_non_strict)
CSP_THROW( ValueError, "Struct '" << s -> meta() -> name() << "' has non-strict inheritance of strict base '" << cur -> name() << "'" );

// rule 2: all local fields are set
std::string missing_fields;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't this need to account for optional struct fields as well?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for all the feedback! Let me know if you spot any other style bugs :)

For this comment, do you mean recursively validating nested strict structs, or checking than an optional field is valid? or am I misunderstanding?

The latter should be implicitly checked by the "Rule 2" loop: to be valid, an optional field should either be given a default or set directly. In either case isSet is true (and false otherwise). All optional fields co-exist in m_fields with the non-optional fields.

The former is a point I wanted to clarify: earlier, I removed recursive validation mostly for performance reasons, assuming any field of strict struct type that is set must've been previously validated (after its creation via meta -> create()).

If created via the Python interface, this should always be the case via tp_init.
From the C++ side, it's enforced manually - as I have it now, all instances of -> create() are followed by a validate(), including in adapters.

If there's future/existing functionality that automatically creates these nested structs w/o immediate validation, recursively validating may be a good move? Maybe there's some other common scenario I'm missing?

Copy link
Collaborator

Choose a reason for hiding this comment

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

"to be valid, an optional field should either be given a default or set directly"

I guess this is the point I am confused on. If I specify a field as Optional[T] with no default, what happens if I leave it unset? Based on Dennis' comment, it will be defaulted to None and then I agree with you, it will be "set" in C++. Then there's no difference between Optional[T] and Optional[T] = None, which I think is fine...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, I see based on your tests that using Optional[T] with no default is equivalent to making the field required. This is confusing to me - what if we just don't allow annotating a field as optional with no default? I don't see a good reason why someone would intentionally do this.

Copy link
Author

@sim15 sim15 Aug 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, I see what you're referring to. Requiring non-defaulted optional fields was my interpretation of the following comment from Rob/the internal discussion notes:

"When opting in, all fields that are not defaulted will be required to be passed into the struct init method. This includes Optional fields that arent defaulted to None."

Was something else meant by this? I thought this was a point of deviation from Dennis' original comment.

In terms of semantics, maybe Dennis' "default optional non-defaulted fields to None" approach is probably more appropriate then? rather than disallowing it

Copy link
Collaborator

@AdamGlustein AdamGlustein Aug 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Semantically, both interpretations of a struct field annotated Optional[T] with no default seem weird to me:

  1. If we default the value to None, that differs from how Python itself handles an Optional annotation. The user should just provide the default value in that case.
  2. If we don't default the value and make it a "strict" field, then it is confusing that they annotated it as Optional[T] and not T. This seems to me that they made a mistake by forgetting the = None part.

So my vote would be to not allow annotating an Optional struct field without a default - @robambalu @p72dennisxu

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess there is a third interpretation here which makes more sense for Optional[T], which is that it's a field that "can be" None but will not be set as None by default. That makes sense to me and aligns with how Python uses the annotation.

Copy link

Choose a reason for hiding this comment

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

I agree, I think on the Python side we can probably just treat it the same as the strict fields, so the user may either provide a default value or set it every time they create a new instance.

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense. Lmk if the this captures these semantics as expected. If we stick with this route, seems like the per-field isOptional bit is not even necessary then, which is a plus. Strictness would enforce that all fields are set after initialization is complete.

for(const auto& field : cur->m_fields) {
if(!field->isSet(s)) {
if(missing_fields.empty()) {
missing_fields = field->fieldname();
} else {
missing_fields += ", " + field->fieldname();
}
}
}

// raise error if any fields are missing
if (!missing_fields.empty()) {
CSP_THROW(ValueError,
"Strict struct '" << cur->name() << "' missing required fields: " << missing_fields);
}
}
}



Struct::Struct( const std::shared_ptr<const StructMeta> & meta )
{
//Initialize meta shared_ptr
Expand Down
45 changes: 29 additions & 16 deletions cpp/csp/engine/Struct.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ class StructField

bool isNative() const { return m_type -> type() <= CspType::Type::MAX_NATIVE_TYPE; }

bool isOptional() const { return m_isOptional; }

void setOffset( size_t off ) { m_offset = off; }
void setMaskOffset( size_t off, uint8_t bit )
{
Expand Down Expand Up @@ -75,7 +77,7 @@ class StructField
protected:

StructField( CspTypePtr type, const std::string & fieldname,
size_t size, size_t alignment );
size_t size, size_t alignment, bool isOptional );

void setIsSet( Struct * s ) const
{
Expand Down Expand Up @@ -108,6 +110,7 @@ class StructField
uint8_t m_maskBit;
uint8_t m_maskBitMask;
CspTypePtr m_type;
const bool m_isOptional;
};

using StructFieldPtr = std::shared_ptr<StructField>;
Expand All @@ -120,7 +123,7 @@ class NativeStructField : public StructField

public:
NativeStructField() {}
NativeStructField( const std::string & fieldname ) : NativeStructField( CspType::fromCType<T>::type(), fieldname )
NativeStructField( const std::string & fieldname, bool isOptional ) : NativeStructField( CspType::fromCType<T>::type(), fieldname, isOptional )
{
}

Expand Down Expand Up @@ -157,7 +160,7 @@ class NativeStructField : public StructField
}

protected:
NativeStructField( CspTypePtr type, const std::string & fieldname ) : StructField( type, fieldname, sizeof( T ), alignof( T ) )
NativeStructField( CspTypePtr type, const std::string & fieldname, bool isOptional ) : StructField( type, fieldname, sizeof( T ), alignof( T ), isOptional )
{}
};

Expand All @@ -179,7 +182,8 @@ using TimeStructField = NativeStructField<Time>;
class CspEnumStructField final : public NativeStructField<CspEnum>
{
public:
CspEnumStructField( CspTypePtr type, const std::string & fieldname ) : NativeStructField( type, fieldname )
CspEnumStructField( CspTypePtr type, const std::string & fieldname, bool isOptional ) : NativeStructField( type,
fieldname, isOptional )
{}
};

Expand Down Expand Up @@ -218,8 +222,8 @@ class NotImplementedStructField : public StructField
class NonNativeStructField : public StructField
{
public:
NonNativeStructField( CspTypePtr type, const std::string &fieldname, size_t size, size_t alignment ) :
StructField( type, fieldname, size, alignment )
NonNativeStructField( CspTypePtr type, const std::string &fieldname, size_t size, size_t alignment, bool isOptional ) :
StructField( type, fieldname, size, alignment, isOptional )
{}

virtual void initialize( Struct * s ) const = 0;
Expand All @@ -244,8 +248,8 @@ class StringStructField final : public NonNativeStructField
public:
using CType = csp::CspType::StringCType;

StringStructField( CspTypePtr type, const std::string & fieldname ) :
NonNativeStructField( type, fieldname, sizeof( CType ), alignof( CType ) )
StringStructField( CspTypePtr type, const std::string & fieldname, bool isOptional ) :
NonNativeStructField( type, fieldname, sizeof( CType ), alignof( CType ), isOptional )
{}

void initialize( Struct * s ) const override
Expand Down Expand Up @@ -343,8 +347,8 @@ class ArrayStructField : public NonNativeStructField
}

public:
ArrayStructField( CspTypePtr arrayType, const std::string & fieldname ) :
NonNativeStructField( arrayType, fieldname, sizeof( CType ), alignof( CType ) )
ArrayStructField( CspTypePtr arrayType, const std::string & fieldname, bool isOptional ) :
NonNativeStructField( arrayType, fieldname, sizeof( CType ), alignof( CType ), isOptional )
{}

const CType & value( const Struct * s ) const
Expand Down Expand Up @@ -421,8 +425,8 @@ class ArrayStructField : public NonNativeStructField
class DialectGenericStructField : public NonNativeStructField
{
public:
DialectGenericStructField( const std::string & fieldname, size_t size, size_t alignment ) :
NonNativeStructField( CspType::DIALECT_GENERIC(), fieldname, size, alignment )
DialectGenericStructField( const std::string & fieldname, size_t size, size_t alignment, bool isOptional ) :
NonNativeStructField( CspType::DIALECT_GENERIC(), fieldname, size, alignment, isOptional )
{}

const DialectGenericType & value( const Struct * s ) const
Expand Down Expand Up @@ -587,21 +591,24 @@ class StructMeta : public std::enable_shared_from_this<StructMeta>
using FieldNames = std::vector<std::string>;

//Fields will be re-arranged and assigned their offsets in StructMeta for optimal performance
StructMeta( const std::string & name, const Fields & fields, std::shared_ptr<StructMeta> base = nullptr );
StructMeta( const std::string & name, const Fields & fields, bool isStrict, std::shared_ptr<StructMeta> base = nullptr );
virtual ~StructMeta();

const std::string & name() const { return m_name; }
size_t size() const { return m_size; }
size_t partialSize() const { return m_partialSize; }

bool isNative() const { return m_isFullyNative; }
bool isStrict() const { return m_isStrict; }

const Fields & fields() const { return m_fields; }
const FieldNames & fieldNames() const { return m_fieldnames; }

size_t maskLoc() const { return m_maskLoc; }
size_t maskSize() const { return m_maskSize; }

void validate( const Struct * s ) const;

const StructFieldPtr & field( const char * name ) const
{
static StructFieldPtr s_empty;
Expand Down Expand Up @@ -652,7 +659,8 @@ class StructMeta : public std::enable_shared_from_this<StructMeta>
std::shared_ptr<StructMeta> m_base;
StructPtr m_default;
FieldMap m_fieldMap;

bool m_isStrict;

//fields in order, memory owners of field objects which in turn own the key memory
//m_fields includes all base fields as well. m_fieldnames maintains the proper iteration order of fields
Fields m_fields;
Expand Down Expand Up @@ -738,6 +746,11 @@ class Struct
return meta() -> allFieldsSet( this );
}

void validate() const
{
meta() -> validate( this );
}


//used to cache dialect representations of this struct, if needed
void * dialectPtr() const { return hidden() -> dialectPtr; }
Expand Down Expand Up @@ -822,8 +835,8 @@ bool TypedStructPtr<T>::operator==( const TypedStructPtr<T> & rhs ) const
class StructStructField final : public NonNativeStructField
{
public:
StructStructField( CspTypePtr cspType, const std::string &fieldname ) :
NonNativeStructField( cspType, fieldname, sizeof( StructPtr ), alignof( StructPtr ) )
StructStructField( CspTypePtr cspType, const std::string &fieldname, bool isOptional ) :
NonNativeStructField( cspType, fieldname, sizeof( StructPtr ), alignof( StructPtr ), isOptional )
{
CSP_ASSERT( cspType -> type() == CspType::Type::STRUCT );
m_meta = std::static_pointer_cast<const CspStructType>( cspType ) -> meta();
Expand Down
Loading
Loading